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

[Martin Packman]

provider/maas: Handle ip_addresses not existing

Older MAAS versions do not return the ip_addresses field in
their instance json. Update the provider to not treat this
as an error, but instead equivalent to having no further
addresses beyond the seperately exposed DNS name.

https://codereview.appspot.com/14543043/

R=fwereade, rogpeppe

provider/maas: Handle ip_addresses not existing

Older MAAS versions do not return the ip_addresses field in
their instance json. Update the provider to not treat this
as an error, but instead equivalent to having no further
addresses beyond the seperately exposed DNS name.

https://codereview.appspot.com/14543043/

[Martin Packman (gz)]

Reviewers: mp+189847_code.launchpad.net,

Message:
Please take a look.

Description:
provider/maas: Handle ip_addresses not existing

Older MAAS versions do not return the ip_addresses field in
their instance json. Update the provider to not treat this
as an error, but instead equivalent to having no further
addresses beyond the seperately exposed DNS name.

https://code.launchpad.net/~gz/juju-core/maas_missing_ip_addresses_1236734/+merge/189847

(do not edit description out of merge proposal)

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

Affected files (+59, -10 lines):
   A [revision details]
   M provider/maas/instance.go
   M provider/maas/instance_test.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-20131008120312-vr5sryx6voukx4o8
+New revision: <email address hidden>

Index: provider/maas/instance.go
=== modified file 'provider/maas/instance.go'
--- provider/maas/instance.go 2013-09-30 19:40:06 +0000
+++ provider/maas/instance.go 2013-10-08 13:29:41 +0000
@@ -76,7 +76,12 @@

  func (mi *maasInstance) ipAddresses() ([]string, error) {
   // we have to do this the hard way, since maasObject doesn't have this
built-in yet
- objs, err := mi.maasObject.GetMap()["ip_addresses"].GetArray()
+ addressArray := mi.maasObject.GetMap()["ip_addresses"]
+ if addressArray.IsNil() {
+ // Older MAAS versions do not return ip_addresses.
+ return nil, nil
+ }
+ objs, err := addressArray.GetArray()
   if err != nil {
    return nil, err
   }

Index: provider/maas/instance_test.go
=== modified file 'provider/maas/instance_test.go'
--- provider/maas/instance_test.go 2013-09-09 07:02:57 +0000
+++ provider/maas/instance_test.go 2013-10-08 13:29:41 +0000
@@ -9,7 +9,6 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/instance"
- jc "launchpad.net/juju-core/testing/checkers"
  )

  type instanceTest struct {
@@ -88,6 +87,12 @@

   expected := []instance.Address{
    {
+ "DNS name",
+ instance.HostName,
+ "",
+ instance.NetworkPublic,
+ },
+ {
     "1.2.3.4",
     instance.Ipv4Address,
     "",
@@ -99,16 +104,53 @@
     "",
     instance.NetworkUnknown,
    },
- {
- "DNS name",
- instance.HostName,
- "",
- instance.NetworkPublic,
- },
   }

   addr, err := inst.Addresses()

- c.Check(err, gc.IsNil)
- c.Check(addr, jc.SameContents, expected)
+ c.Assert(err, gc.IsNil)
+ c.Check(addr, gc.DeepEquals, expected)
+}
+
+func (s *instanceTest) TestAddressesMissing(c *gc.C) {
+ // Older MAAS versions do not have ip_addresses returned, for these
+ // just the DNS name should be returned without error.
+ jsonValue := `{
+ "hostname": "testing.invalid",
+ "system_id": "system_id"
+ }`
+ obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+ inst := maasInstance{&obj, s.environ}
+
+ addr, err := inst.Addresses()
+ c.Assert(err, gc.IsNil)
+ c.Check(addr, gc.DeepEquals, []instance.Address{
+ {"testing.invalid", instance.HostName, "", instance.NetworkPublic},
+ })
+}
+
+func (s *instanceTest) TestAddressesInvalid(c *gc.C) {
+ jsonV...

[Roger Peppe (rogpeppe)]

LGTM with one minor suggestion.

https://codereview.appspot.com/14543043/diff/1/provider/maas/instance_test.go
File provider/maas/instance_test.go (right):

https://codereview.appspot.com/14543043/diff/1/provider/maas/instance_test.go#newcode90
provider/maas/instance_test.go:90: "DNS name",
I'd prefer tagged initialisers for Address here and below.

https://codereview.appspot.com/14543043/

[William Reade (fwereade)]

LGTM, thanks. Can you test this live?

https://codereview.appspot.com/14543043/

=== modified file 'provider/maas/instance.go'
--- provider/maas/instance.go	2013-09-30 19:40:06 +0000
+++ provider/maas/instance.go	2013-10-08 13:58:51 +0000
@@ -76,7 +76,12 @@
 
 func (mi *maasInstance) ipAddresses() ([]string, error) {
 	// we have to do this the hard way, since maasObject doesn't have this built-in yet
-	objs, err := mi.maasObject.GetMap()["ip_addresses"].GetArray()
+	addressArray := mi.maasObject.GetMap()["ip_addresses"]
+	if addressArray.IsNil() {
+		// Older MAAS versions do not return ip_addresses.
+		return nil, nil
+	}
+	objs, err := addressArray.GetArray()
 	if err != nil {
 		return nil, err
 	}

=== modified file 'provider/maas/instance_test.go'
--- provider/maas/instance_test.go	2013-09-09 07:02:57 +0000
+++ provider/maas/instance_test.go	2013-10-08 13:58:51 +0000
@@ -9,7 +9,6 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/instance"
-	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type instanceTest struct {
@@ -79,36 +78,64 @@
 
 func (s *instanceTest) TestAddresses(c *gc.C) {
 	jsonValue := `{
-			"hostname": "DNS name", 
-			"system_id": "system_id", 
+			"hostname": "testing.invalid",
+			"system_id": "system_id",
 			"ip_addresses": [ "1.2.3.4", "fe80::d806:dbff:fe23:1199" ]
 		}`
 	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
 	inst := maasInstance{&obj, s.environ}
 
 	expected := []instance.Address{
-		{
-			"1.2.3.4",
-			instance.Ipv4Address,
-			"",
-			instance.NetworkUnknown,
-		},
-		{
-			"fe80::d806:dbff:fe23:1199",
-			instance.Ipv6Address,
-			"",
-			instance.NetworkUnknown,
-		},
-		{
-			"DNS name",
-			instance.HostName,
-			"",
-			instance.NetworkPublic,
-		},
+		{Value: "testing.invalid", Type: instance.HostName, NetworkScope: instance.NetworkPublic},
+		instance.NewAddress("1.2.3.4"),
+		instance.NewAddress("fe80::d806:dbff:fe23:1199"),
 	}
 
 	addr, err := inst.Addresses()
 
-	c.Check(err, gc.IsNil)
-	c.Check(addr, jc.SameContents, expected)
+	c.Assert(err, gc.IsNil)
+	c.Check(addr, gc.DeepEquals, expected)
+}
+
+func (s *instanceTest) TestAddressesMissing(c *gc.C) {
+	// Older MAAS versions do not have ip_addresses returned, for these
+	// just the DNS name should be returned without error.
+	jsonValue := `{
+		"hostname": "testing.invalid",
+		"system_id": "system_id"
+		}`
+	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+	inst := maasInstance{&obj, s.environ}
+
+	addr, err := inst.Addresses()
+	c.Assert(err, gc.IsNil)
+	c.Check(addr, gc.DeepEquals, []instance.Address{
+		{Value: "testing.invalid", Type: instance.HostName, NetworkScope: instance.NetworkPublic},
+	})
+}
+
+func (s *instanceTest) TestAddressesInvalid(c *gc.C) {
+	jsonValue := `{
+		"hostname": "testing.invalid",
+		"system_id": "system_id",
+		"ip_addresses": "incompatible"
+		}`
+	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+	inst := maasInstance{&obj, s.environ}
+
+	_, err := inst.Addresses()
+	c.Assert(err, gc.NotNil)
+}
+
+func (s *instanceTest) TestAddressesInvalidContents(c *gc.C) {
+	jsonValue := `{
+		"hostname": "testing.invalid",
+		"system_id": "system_id",
+		"ip_addresses": [42]
+		}`
+	obj := s.testMAASObject.TestServer.NewNode(jsonValue)
+	inst := maasInstance{&obj, s.environ}
+
+	_, err := inst.Addresses()
+	c.Assert(err, gc.NotNil)
 }