Merge lp:~jtv/gwacl/name-factory into lp:gwacl

[Jeroen T. Vermeulen]

Export some factories for generating random names.

As discussed with rvba. This encodes knowledge about what exactly can be in the identifier Azure needs for a given type.

The driving use-case is not testing, so much as the need to come up with unique names when working against the real Azure. That's why I didn't go all-out. Of course with this in place, a logical next step would be to export similar factories for other types of identifiers.

Jeroen

[Raphaël Badin (rvb)]

Looks good.

[0]

9 - hostServiceName := makeRandomIdentifier("gwacl", 24)
10 + hostServiceName := gwacl.MakeRandomHostedServiceName("gwacl")

You can probably get rid of the version of makeRandomIdentifier that is in example/management/run.go and use the one from names.go when it's still used directly in run.go.

[1]

79 +// Be sure your random generator is initialized, or this will always produce
80 +// the same sequence!

Why don't we initialize the generator in names.go? This is, I think, so easily forgotten that it's worth not letting that up to the caller.

[2]

56 + if len(prefix) > length {
57 + panic(fmt.Errorf("prefix '%s' is more than the requested %d characters long", prefix, length))
58 + }

I think we're missing a test for that code path.

[3]

75 +// The prefix must be as short as possible, be entirely in ASCII, start with
76 +// a lower-case letter, and contain only lower-case letters and digits after
77 +// that.

Maybe it's me but it's not very clear what "must" means here: is it something that the method checks? I'd add something like "For the prefix to be compatible with Azure's constraints, the prefix must be…" or something.

[4]

81 +func MakeRandomHostedServiceName(prefix string) string {
82 + // A hosted-service name must be between 3 and 24 letters long.
83 + return makeRandomIdentifier(prefix, 24)
84 +}

Could you add links to the relevant piece of documentation please. I'm reading http://msdn.microsoft.com/en-us/library/windowsazure/gg441304.aspx and there is something odd in there:
"""
ServiceName
Required. A name for the cloud service that is unique within Windows Azure. This name is the DNS prefix name and can be used to access the service.
For example: http://ServiceName.cloudapp.net//
"""

This, AFAIK, is just plain wrong :/

review: Approve

[Jeroen T. Vermeulen (jtv)]

You put your finger on an interesting spot. See all the way below.

> 9 - hostServiceName := makeRandomIdentifier("gwacl", 24)
> 10 + hostServiceName := gwacl.MakeRandomHostedServiceName("gwacl")
>
> You can probably get rid of the version of makeRandomIdentifier that is in
> example/management/run.go and use the one from names.go when it's still used
> directly in run.go.

Then I'd have to export it, and update the call sites. But we may still want to mess with makeRandomIdentifier later, e.g. because there may be different kinds of identifier with different rules. I opted to leave as much as I could untouched for now, and not make any promises for a public MakeRandomIdentifier. It'll make much more sense to drop makeRandomIdentifier here once we have replacements for the other calls.

FWIW Pike says duplicating code between packages is the Go way.

> 79 +// Be sure your random generator is initialized, or this will always
> produce
> 80 +// the same sequence!
>
> Why don't we initialize the generator in names.go? This is, I think, so
> easily forgotten that it's worth not letting that up to the caller.

Because for this, I'm perfectly happy with repeated results between contiguous test runs.

> 56 + if len(prefix) > length {
> 57 + panic(fmt.Errorf("prefix '%s' is more than the requested %d
> characters long", prefix, length))
> 58 + }
>
> I think we're missing a test for that code path.

I started to write one, then decided it wasn't worth it. This is outside the function's contract, so the check is lagniappe.

> [3]
>
> 75 +// The prefix must be as short as possible, be entirely in ASCII,
> start with
> 76 +// a lower-case letter, and contain only lower-case letters and
> digits after
> 77 +// that.
>
> Maybe it's me but it's not very clear what "must" means here: is it something
> that the method checks? I'd add something like "For the prefix to be
> compatible with Azure's constraints, the prefix must be…" or something.

No, it's just specifying the contract for this function. If the caller steps outside the contract, they have no right to expect anything.

> 81 +func MakeRandomHostedServiceName(prefix string) string {
> 82 + // A hosted-service name must be between 3 and 24 letters long.
> 83 + return makeRandomIdentifier(prefix, 24)
> 84 +}
>
> Could you add links to the relevant piece of documentation please. I'm
> reading http://msdn.microsoft.com/en-us/library/windowsazure/gg441304.aspx and
> there is something odd in there:
> """
> ServiceName
> Required. A name for the cloud service that is unique within Windows Azure.
> This name is the DNS prefix name and can be used to access the service.
> For example: http://ServiceName.cloudapp.net//
> """
>
> This, AFAIK, is just plain wrong :/

Seriously!? That explains why we got confused over this the other day. What you quote here is the bit that I'd read, and I assumed it made some kind of sense. Sheesh.

But that doesn't explain where the numbers 3 and 24 came from. I think now that that may have been a hurried misinterpretation: a *storage service* has that restric...

[lp:~jtv/gwacl/name-factory]

166. By Jeroen T. Vermeulen on 2013-07-05

No, 3—24 characters was for a storage service name, not a hosted service name.

[Jeroen T. Vermeulen (jtv)]

Oh, if you're talking about initializing the randomizer *inside a call that uses the randomizer*, in production code: please do not do that. It's basically a way to sabotage your randomizer.

For example, say you happen to be using a timer with a resolution is one millisecond. (Exaggerating, but it might be happening somewhere. The timer's API might work in nanoseconds or even picoseconds, but that makes absolutely no promises about actual timer resolution.) You might be resetting your random seed and *repeating the same sequence of random numbers* on every call in the same millisecond!

There may be other patterns as well. Randomizers are carefully designed, and attempts to second-guess them will generally make things worse.

We could keep track of "somebody else may have seeded the randomizer, but has gwacl done it yet?" — but that's a function second-guessing the program's global state. If we impose requirements on the program's global state, the best thing to do is to expose a minimum interface to that state. In this case, it's enough to say "you must have seeded the randomizer if you want fresh pseudorandomness."

[Julian Edwards (julian-edwards)]

On Friday 05 Jul 2013 13:15:33 you wrote:
> 79 +// Be sure your random generator is initialized, or this will
> always produce 80 +// the same sequence!
>
> Why don't we initialize the generator in names.go? This is, I think, so
> easily forgotten that it's worth not letting that up to the caller.

Why not move all of this into helpers_factory_test.go where it really belongs,
which already has *Random* stuff in it and a corresponding init() to
initialise the generator?

[Jeroen T. Vermeulen (jtv)]

It really does not belong there — see the MP: the primary use case is *not* testing. We want to call this from Juju.

[Julian Edwards (julian-edwards)]

On Monday 08 Jul 2013 10:51:35 you wrote:
> It really does not belong there — see the MP: the primary use case is *not*
> testing. We want to call this from Juju.

Oh, missed that. Sorry.

In that case, it belongs in the Juju code.

[Jeroen T. Vermeulen (jtv)]

I don't think it belongs in Juju code. It encodes some very Azure-specific, yet application-agnostic knowledge that other gwacl-using projects might well want to use.

=== modified file 'example/management/run.go'
--- example/management/run.go	2013-07-05 10:33:40 +0000
+++ example/management/run.go	2013-07-05 14:23:22 +0000
@@ -99,8 +99,7 @@
     fmt.Printf("Got image named '%s'\n", sourceImageName)
     fmt.Println("Done getting OS Image\n")
 
-    // A hosted-service name must be between 3 and 24 characters long.
-    hostServiceName := makeRandomIdentifier("gwacl", 24)
+    hostServiceName := gwacl.MakeRandomHostedServiceName("gwacl")
     fmt.Printf("Creating a hosted service: '%s'...\n", hostServiceName)
     createHostedService := gwacl.NewCreateHostedServiceWithLocation(hostServiceName, "testLabel", location)
     err = api.AddHostedService(createHostedService)
@@ -135,7 +134,7 @@
     fmt.Println("Done listing hosted services\n")
 
     fmt.Println("Adding VM deployment...")
-    hostname := makeRandomIdentifier("gwaclhost", 25)
+    hostname := gwacl.MakeRandomHostname("gwaclhost")
     // Random passwords are no use to man nor beast here if you want to
     // test with your instance, so we'll use a fixed one.  It's not really a
     // security hazard in such a short-lived private instance.

=== added file 'names.go'
--- names.go	1970-01-01 00:00:00 +0000
+++ names.go	2013-07-05 14:23:22 +0000
@@ -0,0 +1,74 @@
+// Copyright 2013 Canonical Ltd.  This software is licensed under the
+// GNU Lesser General Public License version 3 (see the file COPYING).
+
+package gwacl
+
+import (
+    "fmt"
+    "math/rand"
+)
+
+// pickOne returns a random choice of one of the characters in chars.
+func pickOne(chars string) string {
+    index := rand.Intn(len(chars))
+    return string(chars[index])
+}
+
+// makeRandomIdentifier creates an arbitrary identifier of the given length,
+// consisting of only ASCII digits and lower-case ASCII letters.
+// The identifier will start with the given prefix.  The prefix must be no
+// longer than the specified length, or there'll be trouble.
+func makeRandomIdentifier(prefix string, length int) string {
+    // Only digits and lower-case ASCII letters are accepted.
+    const (
+        letters = "abcdefghijklmnopqrstuvwxyz"
+        digits  = "0123456789"
+        chars   = letters + digits
+    )
+
+    if len(prefix) > length {
+        panic(fmt.Errorf("prefix '%s' is more than the requested %d characters long", prefix, length))
+    }
+
+    if len(prefix) == 0 {
+        // No prefix.  Still have to start with a letter, so pick one.
+        prefix = pickOne(letters)
+    }
+
+    id := prefix
+    for len(id) < length {
+        id += pickOne(chars)
+    }
+    return id
+}
+
+// MakeRandomHostedServiceName generates a pseudo-random name for a hosted
+// service, with the given prefix.
+//
+// The prefix must be as short as possible, be entirely in ASCII, start with
+// a lower-case letter, and contain only lower-case letters and digits after
+// that.
+//
+// Be sure your random generator is initialized, or this will always produce
+// the same sequence!
+func MakeRandomHostedServiceName(prefix string) string {
+    // We don't know of any documentation on long a hosted-service name can
+    // be, but this is the maximum length that worked in experiments.
+    return makeRandomIdentifier(prefix, 63)
+}
+
+// MakeRandomHostname generates a pseudo-random hostname for a virtual machine,
+// with the given prefix.
+//
+// The prefix must be as short as possible, be entirely in ASCII, start with
+// a lower-case letter, and contain only lower-case letters and digits after
+// that.
+//
+// Be sure your random generator is initialized, or this will always produce
+// the same sequence!
+func MakeRandomHostname(prefix string) string {
+    // Azure documentation says the hostname can be between 1 and 64
+    // letters long, but in practice we found it didn't work with anything
+    // over 55 letters long.
+    return makeRandomIdentifier(prefix, 55)
+}

=== added file 'names_test.go'
--- names_test.go	1970-01-01 00:00:00 +0000
+++ names_test.go	2013-07-05 14:23:22 +0000
@@ -0,0 +1,54 @@
+// Copyright 2013 Canonical Ltd.  This software is licensed under the
+// GNU Lesser General Public License version 3 (see the file COPYING).
+
+package gwacl
+
+import (
+    . "launchpad.net/gocheck"
+    "math/rand"
+)
+
+type namesSuite struct{}
+
+var _ = Suite(&namesSuite{})
+
+func (*namesSuite) TestPickOneReturnsOneCharacter(c *C) {
+    c.Check(len(pickOne("abcd")), Equals, 1)
+}
+
+func (*namesSuite) TestMakeRandomIdentifierObeysLength(c *C) {
+    length := 1 + rand.Intn(50)
+    c.Check(len(makeRandomIdentifier("x", length)), Equals, length)
+}
+
+func (*namesSuite) TestMakeRandomIdentifierRandomizes(c *C) {
+    // There is a minute chance that this will start failing just because
+    // the randomizer repeats a pattern of results.  If so, seed it.
+    c.Check(
+        makeRandomIdentifier("x", 100),
+        Not(Equals),
+        makeRandomIdentifier("x", 100))
+}
+
+func (*namesSuite) TestMakeRandomIdentifierPicksDifferentCharacters(c *C) {
+    // There is a minute chance that this will start failing just because
+    // the randomizer repeats a pattern of results.  If so, seed it.
+    chars := make(map[rune]bool)
+    for _, chr := range makeRandomIdentifier("", 100) {
+        chars[chr] = true
+    }
+    c.Check(len(chars), Not(Equals), 1)
+}
+
+func (*namesSuite) TestMakeRandomIdentifierUsesPrefix(c *C) {
+    c.Check(makeRandomIdentifier("prefix", 10), Matches, "prefix.*")
+}
+
+func (*namesSuite) TestMakeRandomIdentifierUsesOnlyAcceptedCharacters(c *C) {
+    c.Check(makeRandomIdentifier("", 100), Matches, "[0-9a-z]*")
+}
+
+func (*namesSuite) TestMakeRandomIdentifierAcceptsEmptyPrefix(c *C) {
+    // In particular, the first character must still be a letter.
+    c.Check(makeRandomIdentifier("", 1), Matches, "[a-z]")
+}