Merge lp:~gz/juju-core/add_state_address into lp:~go-bot/juju-core/trunk

[Martin Packman]

Add state/address.go for machine location data

Add data structure for describing machine addresses. This is the
first step towards deprecating PublicAddress and PrivateAddress
in unitDoc and storing a list of addresses in machineDoc instead.

The structure uses a single field that may contain either a hostname
or an address. The plan is for a New constructor to correctly flag
the type of location.

https://codereview.appspot.com/11284044/

R=fwereade, jameinel

Add state/address.go for machine location data

Add data structure for describing machine addresses. This is the
first step towards deprecating PublicAddress and PrivateAddress
in unitDoc and storing a list of addresses in machineDoc instead.

The structure uses a single field that may contain either a hostname
or an address. The plan is for a New constructor to correctly flag
the type of location.

Suggestions on naming of things welcome.

https://codereview.appspot.com/11284044/

[Martin Packman (gz)]

Reviewers: mp+174823_code.launchpad.net,

Message:
Please take a look.

Description:
Add state/address.go for machine location data

Add data structure for describing machine addresses. This is the
first step towards deprecating PublicAddress and PrivateAddress
in unitDoc and storing a list of addresses in machineDoc instead.

The structure uses a single field that may contain either a hostname
or an address. The plan is for a New constructor to correctly flag
the type of location.

Suggestions on naming of things welcome.

https://code.launchpad.net/~gz/juju-core/add_state_address/+merge/174823

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11284044/

Affected files:
   A [revision details]
   A state/address.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130715152550-v4p859yzf8o5a3ca
+New revision: <email address hidden>

Index: state/address.go
=== added file 'state/address.go'
--- state/address.go 1970-01-01 00:00:00 +0000
+++ state/address.go 2013-07-15 16:45:38 +0000
@@ -0,0 +1,36 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state
+
+// AddressType represents the possible ways of specifying a machine
location by
+// either a hostname resolvable by dns lookup, or ipv4 or ipv6 address.
+type AddressType string
+
+const (
+ HostName AddressType = "hostname"
+ Ipv4Address AddressType = "ipv4"
+ Ipv6Address AddressType = "ipv6"
+)
+
+// NetworkScope denotes the context a location may apply to. If a name or
+// address can be reached from the wider internet, it is considered
public. A
+// private network address is either specific to the cloud or cloud subnet
a
+// machine belongs to, or to the machine itself for containers.
+type NetworkScope string
+
+const (
+ NetworkUnknown NetworkScope = ""
+ NetworkPublic NetworkScope = "public"
+ NetworkCloudLocal NetworkScope = "local-cloud"
+ NetworkMachineLocal NetworkScope = "local-machine"
+)
+
+// Address represents the location of a machine, including metadata about
what
+// kind of location the address describes.
+type Address struct {
+ Name string
+ Type AddressType
+ NetworkName string `bson:",omitempty"`
+ NetworkScope string `bson:",omitempty"`
+}

[William Reade (fwereade)]

Looks sane to me. If this is intended for merging, LGTM :).

https://codereview.appspot.com/11284044/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/11284044/diff/1/state/address.go#newcode32
state/address.go:32: Name string
Wondering if this is more "Value" than "Name"?

https://codereview.appspot.com/11284044/

[John A Meinel (jameinel)]

How is this linked with the other data structures?

Is it intended that machine will grow an array of Address objects?

I want us to be aware of what happened with adding the container field
(be ready to handle fields that don't exist, as well as ones that have
empty content).

Otherwise LGTM.

I like the Address.Value change proposed by William.

https://codereview.appspot.com/11284044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/11284044/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/11284044/diff/1/state/address.go#newcode25
state/address.go:25: NetworkCloudLocal NetworkScope = "local-cloud"
So a "local-cloud" address is what we currently call a private address?

https://codereview.appspot.com/11284044/diff/1/state/address.go#newcode32
state/address.go:32: Name string
On 2013/07/15 21:50:43, fwereade wrote:
> Wondering if this is more "Value" than "Name"?

I agree here, "Name" seems a bit weird for an IPv4 address.
Seems ok with a hostname, but should really make sense for all.

https://codereview.appspot.com/11284044/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/11284044/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/11284044/diff/1/state/address.go#newcode25
state/address.go:25: NetworkCloudLocal NetworkScope = "local-cloud"
On 2013/07/18 02:38:57, thumper wrote:
> So a "local-cloud" address is what we currently call a private
address?

Yes. Thee public interfaces will still be in terms of "private address"
I just wanted to keep the distinction between a 10. address routed
across a cloud region and a 10. address set up by lxc say.

https://codereview.appspot.com/11284044/diff/1/state/address.go#newcode32
state/address.go:32: Name string
On 2013/07/15 21:50:43, fwereade wrote:
> Wondering if this is more "Value" than "Name"?

Changed.

https://codereview.appspot.com/11284044/

=== added file 'state/address.go'
--- state/address.go	1970-01-01 00:00:00 +0000
+++ state/address.go	2013-07-18 12:40:32 +0000
@@ -0,0 +1,36 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state
+
+// AddressType represents the possible ways of specifying a machine location by
+// either a hostname resolvable by dns lookup, or ipv4 or ipv6 address.
+type AddressType string
+
+const (
+	HostName    AddressType = "hostname"
+	Ipv4Address AddressType = "ipv4"
+	Ipv6Address AddressType = "ipv6"
+)
+
+// NetworkScope denotes the context a location may apply to. If a name or
+// address can be reached from the wider internet, it is considered public. A
+// private network address is either specific to the cloud or cloud subnet a
+// machine belongs to, or to the machine itself for containers.
+type NetworkScope string
+
+const (
+	NetworkUnknown      NetworkScope = ""
+	NetworkPublic       NetworkScope = "public"
+	NetworkCloudLocal   NetworkScope = "local-cloud"
+	NetworkMachineLocal NetworkScope = "local-machine"
+)
+
+// Address represents the location of a machine, including metadata about what
+// kind of location the address describes.
+type Address struct {
+	Value        string
+	Type         AddressType
+	NetworkName  string `bson:",omitempty"`
+	NetworkScope `bson:",omitempty"`
+}

=== added file 'state/address_test.go'
--- state/address_test.go	1970-01-01 00:00:00 +0000
+++ state/address_test.go	2013-07-18 12:40:32 +0000
@@ -0,0 +1,23 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state_test
+
+import (
+	. "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/state"
+)
+
+type AddressSuite struct{}
+
+var _ = Suite(&AddressSuite{})
+
+func (s *AddressSuite) TestMakeAddress(c *C) {
+	addr := state.Address{"0.0.0.0", state.Ipv4Address, "net",
+		state.NetworkUnknown}
+	c.Check(addr.Value, Equals, "0.0.0.0")
+	c.Check(addr.Type, Equals, state.Ipv4Address)
+	c.Check(addr.NetworkName, Equals, "net")
+	c.Check(addr.NetworkScope, Equals, state.NetworkUnknown)
+}