identityservice: out-of-random nil error

identityservice: out-of-random nil error

I was running the test suite on my VM, and I think I
rand out of entropy for a bit. It ended up that I got
a nil pointer dereference exception, which I traced
back to the randomHexToken code incorrectly handling
when it doesn't read enough bytes. (It isn't an error,
it just gets a count smaller than it expected.)

To make it testable, I ended up moving the direct
function call into a pointer that can be overridden.

I also discovered that Go-1.0+ doesn't allow you to compare
function pointers (in case a compiler wants to do
interesting optimizations). So I ended up doing a bit
of a workaround.

It was a longer side trip than I expected, but it did
get me exposed to gocheck.Checkers and some other
reasonable bits.

https://code.launchpad.net/~jameinel/goose/rand-failures/+merge/150751

(do not edit description out of merge proposal)

[jameinel, 2013-02-27 09:56:11]

Please take a look.

[dimitern, 2013-02-27 10:15:31]

LGTM

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
File testservices/identityservice/util_test.go (right):

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util_test.go:35: c.Assert(string(raw), Equals,
"\x00\x00\x00\x00\x00\x00")
I don't think you need this assert here.

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util_test.go:48: func (checker *funcEqualsChecker)
Check(params []interface{}, names []string) (result bool, error string) {
Funky stuff :)

[rog, 2013-02-27 12:19:32]

good to sort this out, but i think a slightly different approach might work out
better.

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
File testservices/identityservice/util.go (right):

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:25: func setRandomizer(r randomizer)
(restore func()) {
it seems like this function should be in the test code.

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:36: n, err := getRandom(raw_bytes)
i'm not sure this is quite right. it wasn't right before, because you should use
io.ReadFull because io.Reader.Read is never guaranteed to return the full number
of bytes.

rather than use getRandom, maybe it might be better to have something like:

var randReader = rand.Reader

then you can use io.ReadFull as expected and there's no need to muck around with
nil errors below.
for the tests you could replace randReader with a custom reader that returns
short reads and/or errors.

[jameinel, 2013-02-28 10:34:59]

Please take a look.

[jameinel, 2013-02-28 10:39:47]

Please take a look.

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
File testservices/identityservice/util.go (right):

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:25: func setRandomizer(r randomizer)
(restore func()) {
On 2013/02/27 12:19:32, rog wrote:
> it seems like this function should be in the test code.

It is used in testing, but it still seems more like general functionality.

https://codereview.appspot.com/7406054/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:36: n, err := getRandom(raw_bytes)
Thanks for the pointer to this.
Looking here: http://golang.org/pkg/crypto/rand/

I realize I didn't see it before because the Reader object *isn't* in the Index
overview. (It shows up under Variables in a gray box which makes me think it is
a code example, and not a core data structure that you should be using.)

I did make this change, so please give it another look.

[rog, 2013-02-28 11:36:15]

better, thanks, but i think it can be simpler still.

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
File testservices/identityservice/util.go (right):

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:24: func setReader(r io.Reader) (restore
func()) {
i still think this should be in the test code only. it's unexported, unsafe to
use in general, and only used by the test code (and if anything in this package
*did* want to do this, it could easily just set randReader, as in fact the test
could)

you've got more test code for setReader than you have for the actual code you
want to test!

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:36: if n != 16 || err != nil {
if err != nil {
is sufficient, as ReadFull is guaranteed to return an error if it doesn't read
the correct amount of data.
then you can lose the if below.

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:42: "Could not read 16 random bytes safely
(read %d bytes): %s",
what does "safely" mean here?

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:45: hex_bytes := make([]byte, 32)
s/32/hex.EncodedLen(len(raw_bytes))

?

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:47: if n != 32 || err != nil {
if you use EncodedLen, then there's no need to do this check
(see http://golang.org/pkg/encoding/hex/#Encode)

the err check is spurious too, as hex.Encode doesn't return an error.

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
File testservices/identityservice/util_test.go (right):

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util_test.go:34: func (s *UtilSuite)
TestDefaultReader(c *C) {
i'm not really convinced this is checking anything that's not checked by
c.Assert(randReader, Equals, rand.Reader)

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util_test.go:84: }
for completeness, you could have a test that checks with iotest.OneByteReader -
that test would have failed before and an incomplete read is the most likely
scenario (i don't *think* /dev/urandom ever actually refuses to return any data
at all).

[jameinel, 2013-02-28 13:04:25]

Please take a look.

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
File testservices/identityservice/util.go (right):

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util.go:47: if n != 32 || err != nil {
> the err check is spurious too, as hex.Encode doesn't return an error.

Sure, that was probably carry over.

As for EncodedLen, if the definition of 'hex' ever changes (from 2 hex bytes for
every raw byte), we'll be pretty borked. If you prefer I can just ignore n and
err always. It is pretty edge-cased that it would ever not be correct.

Otherwise the point of checking 'n' is to see if it actually converted the right
number of bytes to hex. If we just say 'hex.Encode cannot fail' then we can just
treat it that way.

[rog, 2013-02-28 15:06:56]

much better thanks. LGTM with a few remaining thoughts below.

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
File testservices/identityservice/util_test.go (right):

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util_test.go:84: }
On 2013/02/28 11:36:15, rog wrote:
> for completeness, you could have a test that checks with iotest.OneByteReader
-
> that test would have failed before and an incomplete read is the most likely
> scenario (i don't *think* /dev/urandom ever actually refuses to return any
data
> at all).

it would be nice to see this test in place - testing the actual functionality
that failed in the first place.

https://codereview.appspot.com/7406054/diff/14001/testservices/identityservic...
File testservices/identityservice/util_test.go (right):

https://codereview.appspot.com/7406054/diff/14001/testservices/identityservic...
testservices/identityservice/util_test.go:43: func (s *UtilSuite)
TestSetReader(c *C) {
we don't usually test the test-infrastructure itself - if setReader doesn't
work, the tests will fail.

same goes for TestDefaultReader above - an equivalent (and simpler and more
direct) test would be c.Assert(randomHexToken() != strings.Repeat("0", 32))

that said, if you really feel these tests are worth the lines of code they take,
then leave them in.

https://codereview.appspot.com/7406054/diff/14001/testservices/identityservic...
testservices/identityservice/util_test.go:66: // Note: This is not thread safe,
but it is really only for testing anyway
this last line seems unnecessary since this is in a _test.go file.

[jameinel, 2013-03-08 21:54:54]

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
File testservices/identityservice/util_test.go (right):

https://codereview.appspot.com/7406054/diff/7001/testservices/identityservice...
testservices/identityservice/util_test.go:84: }
added

[jameinel, 2013-03-08 21:59:29]

Please take a look.