Code review comment for lp:~gz/juju-core/api_endpoint_no_machine_local_1310258

Reviewers: mp+216744_code.launchpad.net,

Message:
Please take a look.

Description:
juju: Omit IPv6 and local addresses from jenv

Until the jenv addresses cache gains the extra metadata
fields that are returned over the API, filter out the
addresses that are unlikely to be useful for connecting
to an API server.

https://code.launchpad.net/~gz/juju-core/api_endpoint_no_machine_local_1310258/+merge/216744

(do not edit description out of merge proposal)

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

Affected files (+42, -1 lines):
   A [revision details]
   M juju/api.go
   M juju/apiconn_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-20140422051657-1svpzqvmysbn4tqm
+New revision: <email address hidden>

Index: juju/api.go
=== modified file 'juju/api.go'
--- juju/api.go 2014-04-17 15:35:11 +0000
+++ juju/api.go 2014-04-22 15:00:21 +0000
@@ -353,7 +353,11 @@
   var addrs []string
   for _, serverHostPorts := range st.APIHostPorts() {
    for _, hostPort := range serverHostPorts {
- addrs = append(addrs, hostPort.NetAddr())
+ // Only cache addresses that are likely to be usable,
+ // exclude IPv6 for now and localhost style ones.
+ if hostPort.Type != instance.Ipv6Address && hostPort.NetworkScope !=
instance.NetworkMachineLocal {
+ addrs = append(addrs, hostPort.NetAddr())
+ }
    }
   }
   endpoint := info.APIEndpoint()

Index: juju/apiconn_test.go
=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go 2014-04-17 15:35:11 +0000
+++ juju/apiconn_test.go 2014-04-22 14:34:29 +0000
@@ -661,3 +661,38 @@
   // This refresh now gives us the values return by APIHostPorts
   c.Check(endpoint.Addresses, gc.DeepEquals, []string{"0.1.2.3:1234"})
  }
+
+func (s *APIEndpointForEnvSuite) TestAPIEndpointNotMachineLocal(c *gc.C) {
+ defer coretesting.MakeEmptyFakeHome(c).Restore()
+ store := newConfigStore("env-name", dummyStoreInfo)
+ called := 0
+ hostPorts := [][]instance.HostPort{
+ instance.AddressesWithPort([]instance.Address{
+ instance.NewAddress("1.0.0.1", instance.NetworkPublic),
+ instance.NewAddress("192.0.0.1", instance.NetworkCloudLocal),
+ instance.NewAddress("127.0.0.1", instance.NetworkMachineLocal),
+ instance.NewAddress("localhost", instance.NetworkMachineLocal),
+ }, 1234),
+ instance.AddressesWithPort([]instance.Address{
+ instance.NewAddress("1.0.0.2", instance.NetworkUnknown),
+ instance.NewAddress("2002:0:0:0:0:0:100:2", instance.NetworkUnknown),
+ instance.NewAddress("::1", instance.NetworkMachineLocal),
+ instance.NewAddress("127.0.0.1", instance.NetworkMachineLocal),
+ instance.NewAddress("localhost", instance.NetworkMachineLocal),
+ }, 1235),
+ }
+
+ expectState := &mockAPIState{apiHostPorts: hostPorts}
+ apiOpen := func(_ *api.Info, _ api.DialOpts) (juju.APIState, error) {
+ called++
+ return expectState, nil
+ }
+ endpoint, err := juju.APIEndpointInStore("env-name", true, store, apiOpen)
+ c.Assert(err, gc.IsNil)
+ c.Check(called, gc.Equals, 1)
+ c.Check(endpoint.Addresses, gc.DeepEquals, []string{
+ "1.0.0.1:1234",
+ "192.0.0.1:1234",
+ "1.0.0.2:1235",
+ })
+}