juju-core

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

  1. api_endpoint_no_machine_local_1310258
  2. Merge into trunk
Proposed by Martin Packman
Status: Merged
Approved by: Ian Booth
Approved revision: no longer in the source branch.
Merged at revision: 2664
Proposed branch: lp:~gz/juju-core/api_endpoint_no_machine_local_1310258
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 59 lines (+40/-1)
2 files modified
juju/api.go (+5/-1)
juju/apiconn_test.go (+35/-0)
To merge this branch: bzr merge lp:~gz/juju-core/api_endpoint_no_machine_local_1310258
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+216744@code.launchpad.net

Commit message

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://codereview.appspot.com/90310043/

Description of the change

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://codereview.appspot.com/90310043/

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :
Download full text (3.2 KiB)

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.Ad...

Read more...

Revision history for this message
Ian Booth (wallyworld) wrote :

I tested this with local provider and got:

- 10.0.1.1:17070
- localhost:17070
- 192.168.1.116:17070
- 172.16.48.1:17070
- 172.16.175.1:17070

So localhost is still included which is kinda cool because that's where
the state server runs for local provider. So even though the bug says
localhost should be excluded that's not entirely accurate.

LGTM

https://codereview.appspot.com/90310043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (10.6 KiB)

The attempt to merge lp:~gz/juju-core/api_endpoint_no_machine_local_1310258 into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.862s
ok launchpad.net/juju-core/agent/mongo 1.339s
ok launchpad.net/juju-core/agent/tools 0.223s
ok launchpad.net/juju-core/bzr 5.079s
ok launchpad.net/juju-core/cert 2.943s
ok launchpad.net/juju-core/charm 0.466s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 0.802s
ok launchpad.net/juju-core/cmd 0.164s
ok launchpad.net/juju-core/cmd/charm-admin 0.760s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/envcmd 0.157s
ok launchpad.net/juju-core/cmd/juju 225.952s
ok launchpad.net/juju-core/cmd/jujud 76.864s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 7.432s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.136s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.023s
ok launchpad.net/juju-core/container 0.035s
ok launchpad.net/juju-core/container/factory 0.042s
ok launchpad.net/juju-core/container/kvm 0.208s
ok launchpad.net/juju-core/container/kvm/mock 0.045s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.309s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.217s
ok launchpad.net/juju-core/environs 2.286s
ok launchpad.net/juju-core/environs/bootstrap 12.359s
ok launchpad.net/juju-core/environs/cloudinit 0.487s
ok launchpad.net/juju-core/environs/config 3.257s
ok launchpad.net/juju-core/environs/configstore 0.033s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 0.691s
ok launchpad.net/juju-core/environs/imagemetadata 0.493s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.044s
ok launchpad.net/juju-core/environs/jujutest 0.185s
ok launchpad.net/juju-core/environs/manual 15.139s
? launchpad.net/juju-core/environs/network [no test files]
ok launchpad.net/juju-core/environs/simplestreams 0.277s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.920s
ok launchpad.net/juju-core/environs/storage 0.935s
ok launchpad.net/juju-core/environs/sync 50.183s
ok launchpad.net/juju-core/environs/testing 0.151s
ok launchpad.net/juju-core/environs/tools 4.610s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.017s
? launchpad.net/juju-core/...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'juju/api.go'
2--- juju/api.go 2014-04-17 15:35:11 +0000
3+++ juju/api.go 2014-04-22 15:08:23 +0000
4@@ -353,7 +353,11 @@
5 var addrs []string
6 for _, serverHostPorts := range st.APIHostPorts() {
7 for _, hostPort := range serverHostPorts {
8- addrs = append(addrs, hostPort.NetAddr())
9+ // Only cache addresses that are likely to be usable,
10+ // exclude IPv6 for now and localhost style ones.
11+ if hostPort.Type != instance.Ipv6Address && hostPort.NetworkScope != instance.NetworkMachineLocal {
12+ addrs = append(addrs, hostPort.NetAddr())
13+ }
14 }
15 }
16 endpoint := info.APIEndpoint()
17
18=== modified file 'juju/apiconn_test.go'
19--- juju/apiconn_test.go 2014-04-17 15:35:11 +0000
20+++ juju/apiconn_test.go 2014-04-22 15:08:23 +0000
21@@ -661,3 +661,38 @@
22 // This refresh now gives us the values return by APIHostPorts
23 c.Check(endpoint.Addresses, gc.DeepEquals, []string{"0.1.2.3:1234"})
24 }
25+
26+func (s *APIEndpointForEnvSuite) TestAPIEndpointNotMachineLocal(c *gc.C) {
27+ defer coretesting.MakeEmptyFakeHome(c).Restore()
28+ store := newConfigStore("env-name", dummyStoreInfo)
29+ called := 0
30+ hostPorts := [][]instance.HostPort{
31+ instance.AddressesWithPort([]instance.Address{
32+ instance.NewAddress("1.0.0.1", instance.NetworkPublic),
33+ instance.NewAddress("192.0.0.1", instance.NetworkCloudLocal),
34+ instance.NewAddress("127.0.0.1", instance.NetworkMachineLocal),
35+ instance.NewAddress("localhost", instance.NetworkMachineLocal),
36+ }, 1234),
37+ instance.AddressesWithPort([]instance.Address{
38+ instance.NewAddress("1.0.0.2", instance.NetworkUnknown),
39+ instance.NewAddress("2002:0:0:0:0:0:100:2", instance.NetworkUnknown),
40+ instance.NewAddress("::1", instance.NetworkMachineLocal),
41+ instance.NewAddress("127.0.0.1", instance.NetworkMachineLocal),
42+ instance.NewAddress("localhost", instance.NetworkMachineLocal),
43+ }, 1235),
44+ }
45+
46+ expectState := &mockAPIState{apiHostPorts: hostPorts}
47+ apiOpen := func(_ *api.Info, _ api.DialOpts) (juju.APIState, error) {
48+ called++
49+ return expectState, nil
50+ }
51+ endpoint, err := juju.APIEndpointInStore("env-name", true, store, apiOpen)
52+ c.Assert(err, gc.IsNil)
53+ c.Check(called, gc.Equals, 1)
54+ c.Check(endpoint.Addresses, gc.DeepEquals, []string{
55+ "1.0.0.1:1234",
56+ "192.0.0.1:1234",
57+ "1.0.0.2:1235",
58+ })
59+}

Subscribers

People subscribed via source and target branches

to status/vote changes: