Merge lp:~axwalk/juju-core/lp1225825-netlookupip-ip into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 1831
Proposed branch: lp:~axwalk/juju-core/lp1225825-netlookupip-ip
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 320 lines (+116/-49)
3 files modified
environs/manual/provisioner.go (+13/-0)
instance/address.go (+20/-6)
instance/address_test.go (+83/-43)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1225825-netlookupip-ip
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+186229@code.launchpad.net

Commit message

environs/manual: reverse lookup target IP

Thus we may resolve additional addresses for the
machine. Also, we should get a consistent machine ID,
whether a name or an IP is specified.

Also, modify instance/HostAddresses to avoid passing
an IP address into net.LookupIP. This causes an error
if built with CGO_ENABLED=0.

Fixes #1225825

https://codereview.appspot.com/13504044/

Description of the change

environs/manual: reverse lookup target IP

Thus we may resolve additional addresses for the
machine. Also, we should get a consistent machine ID,
whether a name or an IP is specified.

Also, modify instance/HostAddresses to avoid passing
an IP address into net.LookupIP. This causes an error
if built with CGO_ENABLED=0.

Fixes #1225825

https://codereview.appspot.com/13504044/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+186229_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: reverse lookup target IP

Thus we may resolve additional addresses for the
machine. Also, we should get a consistent machine ID,
whether a name or an IP is specified.

Also, modify instance/HostAddresses to avoid passing
an IP address into net.LookupIP. This causes an error
if built with CGO_ENABLED=0.

Fixes #1225825

https://code.launchpad.net/~axwalk/juju-core/lp1225825-netlookupip-ip/+merge/186229

(do not edit description out of merge proposal)

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

Affected files (+23, -2 lines):
   A [revision details]
   M environs/manual/provisioner.go
   M instance/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-20130917082542-j1hu82fubd89ombo
+New revision: <email address hidden>

Index: instance/address.go
=== modified file 'instance/address.go'
--- instance/address.go 2013-09-04 15:11:51 +0000
+++ instance/address.go 2013-09-18 02:36:52 +0000
@@ -61,12 +61,17 @@

  // HostAddresses looks up the IP addresses of the specified
  // host, and translates them into instance.Address values.
-func HostAddresses(host string) ([]Address, error) {
+func HostAddresses(host string) (addrs []Address, err error) {
+ hostAddr := NewAddress(host)
+ if hostAddr.Type != HostName {
+ // IPs shouldn't be fed into LookupIP.
+ return []Address{hostAddr}, nil
+ }
   ipaddrs, err := net.LookupIP(host)
   if err != nil {
    return nil, err
   }
- addrs := make([]Address, len(ipaddrs))
+ addrs = make([]Address, len(ipaddrs)+1)
   for i, ipaddr := range ipaddrs {
    switch len(ipaddr) {
    case 4:
@@ -77,6 +82,7 @@
     addrs[i].Value = ipaddr.String()
    }
   }
+ addrs[len(addrs)-1] = hostAddr
   return addrs, err
  }

Index: environs/manual/provisioner.go
=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go 2013-09-03 01:56:25 +0000
+++ environs/manual/provisioner.go 2013-09-18 02:36:52 +0000
@@ -6,6 +6,7 @@
  import (
   "errors"
   "fmt"
+ "net"
   "strings"

   "launchpad.net/loggo"
@@ -71,10 +72,22 @@
   if at := strings.Index(sshHostWithoutUser, "@"); at != -1 {
    sshHostWithoutUser = sshHostWithoutUser[at+1:]
   }
+ if ip := net.ParseIP(sshHostWithoutUser); ip != nil {
+ // Do a reverse-lookup on the IP. The IP may not have
+ // a DNS entry, so just log a warning if this fails.
+ names, err := net.LookupAddr(ip.String())
+ if err != nil {
+ logger.Warningf("failed to resolve %v: %v", ip, err)
+ } else {
+ logger.Infof("resolved %v to %v", ip, names)
+ sshHostWithoutUser = names[0]
+ }
+ }
   addrs, err := instance.HostAddresses(sshHostWithoutUser)
   if err != nil {
    return nil, err
   }
+ logger.Infof("addresses for %v: %v", sshHostWithoutUser, addrs)

   provisioned, err := checkProvisioned(args.Host)
   if err != nil {

Revision history for this message
Tim Penhey (thumper) wrote :

LGTM with a test added, also consider the logging severity.

https://codereview.appspot.com/13504044/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/13504044/diff/1/environs/manual/provisioner.go#newcode80
environs/manual/provisioner.go:80: logger.Warningf("failed to resolve
%v: %v", ip, err)
Is it really a warning if we can't do a reverse lookup? Is it something
that we should be fixing later?

Perhaps just log with info or debug, certainly not really a big deal is
it?

https://codereview.appspot.com/13504044/diff/1/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/13504044/diff/1/instance/address.go#newcode64
instance/address.go:64: func HostAddresses(host string) (addrs
[]Address, err error) {
How about a test for this?

https://codereview.appspot.com/13504044/

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/13504044/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/13504044/diff/1/environs/manual/provisioner.go#newcode80
environs/manual/provisioner.go:80: logger.Warningf("failed to resolve
%v: %v", ip, err)
On 2013/09/18 03:52:12, thumper wrote:
> Is it really a warning if we can't do a reverse lookup? Is it
something that we
> should be fixing later?

Hmm, yeah, it's not necessarily a problem.

> Perhaps just log with info or debug, certainly not really a big deal
is it?

I'll log it as INFO.

https://codereview.appspot.com/13504044/

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

The attempt to merge lp:~axwalk/juju-core/lp1225825-netlookupip-ip into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.733s
ok launchpad.net/juju-core/agent/tools 0.297s
ok launchpad.net/juju-core/bzr 8.078s
ok launchpad.net/juju-core/cert 2.532s
ok launchpad.net/juju-core/charm 0.603s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.213s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddUnitSuite.SetUpTest

... Panic: Couldn't create temporary directory: mkdir /tmp/gocheck-5765484004404056823: file exists (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/home/tarmac/trees/src/launchpad.net/gocheck/gocheck.go:137
  in tempDir.newPath
/home/tarmac/trees/src/launchpad.net/gocheck/gocheck.go:159
  in C.MkDir
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:128
  in JujuConnSuite.SetUpTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/repo.go:27
  in RepoSuite.SetUpTest

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddUnitSuite.TearDownTest

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/usr/lib/go/src/pkg/runtime/panic.c:487
  in panicstring
/usr/lib/go/src/pkg/runtime/os_linux.c:236
  in sigpanic
/home/tarmac/trees/src/launchpad.net/juju-core/state/state.go:1050
  in State.SetAdminMongoPassword
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:280
  in JujuConnSuite.tearDownConn
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/conn.go:136
  in JujuConnSuite.TearDownTest
/home/tarmac/trees/src/launchpad.net/juju-core/juju/testing/repo.go:46
  in RepoSuite.TearDownTest

----------------------------------------------------------------------
PANIC: addunit_test.go:60: AddUnitSuite.TestAddUnit

... Panic: Fixture has panicked (see related PANIC)
listing available tools
found 4 tools
found 4 recent tools (version 1.2.0)
listing target bucket
found 3 tools in target; 4 tools to be copied
copying 1.2.0-precise-amd64 from http://127.0.0.1:40557/tools/juju-1.2.0-precise-amd64.tgz
copying tools/releases/juju-1.2.0-precise-amd64.tgz
downloaded tools/releases/juju-1.2.0-precise-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.2.0-quantal-amd64 from http://127.0.0.1:40557/tools/juju-1.2.0-quantal-amd64.tgz
copying tools/releases/juju-1.2.0-quantal-amd64.tgz
downloaded tools/releases/juju-1.2.0-quantal-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.2.0-quantal-i386 from http://127.0.0.1:40557/tools/juju-1.2.0-quantal-i386.tgz
copying tools/releases/juju-1.2.0-quantal-i386.tgz
downloaded tools/releases/juju-1.2.0-quantal-i386.tgz (0kB), uploading
download 0kB, uploading
copying 1.2.0-raring-amd64 from http://127.0.0.1:40557/tools/juju-1.2.0-raring-...

Revision history for this message
John A Meinel (jameinel) wrote :

The bot failure is because go check doesn't seed its random-directory generator, and we hit 100 leaked directories.

I think we have a bug somewhere that is causing us to not clean up a dir during testing, but unfortunately I thought of that right after I nuked all the dirs, so it will be hard to figure out where until this happens again.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Thanks jam. When I have my MPs in order, I'll run some tests locally to see if I can find the culprits.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/manual/provisioner.go'
2--- environs/manual/provisioner.go 2013-09-03 01:56:25 +0000
3+++ environs/manual/provisioner.go 2013-09-18 04:44:17 +0000
4@@ -6,6 +6,7 @@
5 import (
6 "errors"
7 "fmt"
8+ "net"
9 "strings"
10
11 "launchpad.net/loggo"
12@@ -71,10 +72,22 @@
13 if at := strings.Index(sshHostWithoutUser, "@"); at != -1 {
14 sshHostWithoutUser = sshHostWithoutUser[at+1:]
15 }
16+ if ip := net.ParseIP(sshHostWithoutUser); ip != nil {
17+ // Do a reverse-lookup on the IP. The IP may not have
18+ // a DNS entry, so just log a warning if this fails.
19+ names, err := net.LookupAddr(ip.String())
20+ if err != nil {
21+ logger.Infof("failed to resolve %v: %v", ip, err)
22+ } else {
23+ logger.Infof("resolved %v to %v", ip, names)
24+ sshHostWithoutUser = names[0]
25+ }
26+ }
27 addrs, err := instance.HostAddresses(sshHostWithoutUser)
28 if err != nil {
29 return nil, err
30 }
31+ logger.Infof("addresses for %v: %v", sshHostWithoutUser, addrs)
32
33 provisioned, err := checkProvisioned(args.Host)
34 if err != nil {
35
36=== modified file 'instance/address.go'
37--- instance/address.go 2013-09-04 15:11:51 +0000
38+++ instance/address.go 2013-09-18 04:44:17 +0000
39@@ -59,24 +59,38 @@
40 return Address{value, addresstype, "", NetworkUnknown}
41 }
42
43+// netLookupIP is a var for testing.
44+var netLookupIP = net.LookupIP
45+
46 // HostAddresses looks up the IP addresses of the specified
47 // host, and translates them into instance.Address values.
48-func HostAddresses(host string) ([]Address, error) {
49- ipaddrs, err := net.LookupIP(host)
50+func HostAddresses(host string) (addrs []Address, err error) {
51+ hostAddr := NewAddress(host)
52+ if hostAddr.Type != HostName {
53+ // IPs shouldn't be fed into LookupIP.
54+ return []Address{hostAddr}, nil
55+ }
56+ ipaddrs, err := netLookupIP(host)
57 if err != nil {
58 return nil, err
59 }
60- addrs := make([]Address, len(ipaddrs))
61+ addrs = make([]Address, len(ipaddrs)+1)
62 for i, ipaddr := range ipaddrs {
63 switch len(ipaddr) {
64- case 4:
65+ case net.IPv4len:
66 addrs[i].Type = Ipv4Address
67 addrs[i].Value = ipaddr.String()
68- case 16:
69- addrs[i].Type = Ipv6Address
70+ case net.IPv6len:
71+ if ipaddr.To4() != nil {
72+ // ipaddr is an IPv4 address represented in 16 bytes.
73+ addrs[i].Type = Ipv4Address
74+ } else {
75+ addrs[i].Type = Ipv6Address
76+ }
77 addrs[i].Value = ipaddr.String()
78 }
79 }
80+ addrs[len(addrs)-1] = hostAddr
81 return addrs, err
82 }
83
84
85=== modified file 'instance/address_test.go'
86--- instance/address_test.go 2013-09-05 16:02:35 +0000
87+++ instance/address_test.go 2013-09-18 04:44:17 +0000
88@@ -1,12 +1,15 @@
89 // Copyright 2013 Canonical Ltd.
90 // Licensed under the AGPLv3, see LICENCE file for details.
91
92-package instance_test
93+package instance
94
95 import (
96+ "errors"
97+ "net"
98+
99 gc "launchpad.net/gocheck"
100
101- "launchpad.net/juju-core/instance"
102+ jc "launchpad.net/juju-core/testing/checkers"
103 )
104
105 type AddressSuite struct{}
106@@ -14,64 +17,64 @@
107 var _ = gc.Suite(&AddressSuite{})
108
109 func (s *AddressSuite) TestNewAddressIpv4(c *gc.C) {
110- addr := instance.NewAddress("127.0.0.1")
111+ addr := NewAddress("127.0.0.1")
112 c.Check(addr.Value, gc.Equals, "127.0.0.1")
113- c.Check(addr.Type, gc.Equals, instance.Ipv4Address)
114+ c.Check(addr.Type, gc.Equals, Ipv4Address)
115 }
116
117 func (s *AddressSuite) TestNewAddressIpv6(c *gc.C) {
118- addr := instance.NewAddress("::1")
119+ addr := NewAddress("::1")
120 c.Check(addr.Value, gc.Equals, "::1")
121- c.Check(addr.Type, gc.Equals, instance.Ipv6Address)
122+ c.Check(addr.Type, gc.Equals, Ipv6Address)
123 }
124
125 func (s *AddressSuite) TestNewAddressHostname(c *gc.C) {
126- addr := instance.NewAddress("localhost")
127+ addr := NewAddress("localhost")
128 c.Check(addr.Value, gc.Equals, "localhost")
129- c.Check(addr.Type, gc.Equals, instance.HostName)
130+ c.Check(addr.Type, gc.Equals, HostName)
131 }
132
133 type selectTests struct {
134 about string
135- addresses []instance.Address
136+ addresses []Address
137 expected string
138 }
139
140 var selectPublicTests = []selectTests{{
141 "no addresses gives empty string result",
142- []instance.Address{},
143+ []Address{},
144 "",
145 }, {
146 "a public address is selected",
147- []instance.Address{
148- {"8.8.8.8", instance.Ipv4Address, "public", instance.NetworkPublic},
149+ []Address{
150+ {"8.8.8.8", Ipv4Address, "public", NetworkPublic},
151 },
152 "8.8.8.8",
153 }, {
154 "a machine local address is not selected",
155- []instance.Address{
156- {"127.0.0.1", instance.Ipv4Address, "machine", instance.NetworkMachineLocal},
157+ []Address{
158+ {"127.0.0.1", Ipv4Address, "machine", NetworkMachineLocal},
159 },
160 "",
161 }, {
162 "an ipv6 address is not selected",
163- []instance.Address{
164- {"2001:DB8::1", instance.Ipv6Address, "", instance.NetworkPublic},
165+ []Address{
166+ {"2001:DB8::1", Ipv6Address, "", NetworkPublic},
167 },
168 "",
169 }, {
170 "a public name is preferred to an unknown or cloud local address",
171- []instance.Address{
172- {"127.0.0.1", instance.Ipv4Address, "local", instance.NetworkUnknown},
173- {"10.0.0.1", instance.Ipv4Address, "cloud", instance.NetworkCloudLocal},
174- {"public.invalid.testing", instance.HostName, "public", instance.NetworkPublic},
175+ []Address{
176+ {"127.0.0.1", Ipv4Address, "local", NetworkUnknown},
177+ {"10.0.0.1", Ipv4Address, "cloud", NetworkCloudLocal},
178+ {"public.invalid.testing", HostName, "public", NetworkPublic},
179 },
180 "public.invalid.testing",
181 }, {
182 "last unknown address selected",
183- []instance.Address{
184- {"10.0.0.1", instance.Ipv4Address, "cloud", instance.NetworkUnknown},
185- {"8.8.8.8", instance.Ipv4Address, "floating", instance.NetworkUnknown},
186+ []Address{
187+ {"10.0.0.1", Ipv4Address, "cloud", NetworkUnknown},
188+ {"8.8.8.8", Ipv4Address, "floating", NetworkUnknown},
189 },
190 "8.8.8.8",
191 }}
192@@ -79,43 +82,43 @@
193 func (s *AddressSuite) TestSelectPublicAddressEmpty(c *gc.C) {
194 for i, t := range selectPublicTests {
195 c.Logf("test %d. %s", i, t.about)
196- c.Check(instance.SelectPublicAddress(t.addresses), gc.Equals, t.expected)
197+ c.Check(SelectPublicAddress(t.addresses), gc.Equals, t.expected)
198 }
199 }
200
201 var selectInternalTests = []selectTests{{
202 "no addresses gives empty string result",
203- []instance.Address{},
204+ []Address{},
205 "",
206 }, {
207 "a public address is selected",
208- []instance.Address{
209- {"8.8.8.8", instance.Ipv4Address, "public", instance.NetworkPublic},
210+ []Address{
211+ {"8.8.8.8", Ipv4Address, "public", NetworkPublic},
212 },
213 "8.8.8.8",
214 }, {
215 "a cloud local address is selected",
216- []instance.Address{
217- {"10.0.0.1", instance.Ipv4Address, "private", instance.NetworkCloudLocal},
218+ []Address{
219+ {"10.0.0.1", Ipv4Address, "private", NetworkCloudLocal},
220 },
221 "10.0.0.1",
222 }, {
223 "a machine local address is not selected",
224- []instance.Address{
225- {"127.0.0.1", instance.Ipv4Address, "machine", instance.NetworkMachineLocal},
226+ []Address{
227+ {"127.0.0.1", Ipv4Address, "machine", NetworkMachineLocal},
228 },
229 "",
230 }, {
231 "ipv6 addresses are not selected",
232- []instance.Address{
233- {"::1", instance.Ipv6Address, "", instance.NetworkCloudLocal},
234+ []Address{
235+ {"::1", Ipv6Address, "", NetworkCloudLocal},
236 },
237 "",
238 }, {
239 "a cloud local address is preferred to a public address",
240- []instance.Address{
241- {"10.0.0.1", instance.Ipv4Address, "cloud", instance.NetworkCloudLocal},
242- {"8.8.8.8", instance.Ipv4Address, "public", instance.NetworkPublic},
243+ []Address{
244+ {"10.0.0.1", Ipv4Address, "cloud", NetworkCloudLocal},
245+ {"8.8.8.8", Ipv4Address, "public", NetworkPublic},
246 },
247 "10.0.0.1",
248 }}
249@@ -123,20 +126,20 @@
250 func (s *AddressSuite) TestSelectInternalAddress(c *gc.C) {
251 for i, t := range selectInternalTests {
252 c.Logf("test %d. %s", i, t.about)
253- c.Check(instance.SelectInternalAddress(t.addresses, false), gc.Equals, t.expected)
254+ c.Check(SelectInternalAddress(t.addresses, false), gc.Equals, t.expected)
255 }
256 }
257
258 var selectInternalMachineTests = []selectTests{{
259 "a cloud local address is selected",
260- []instance.Address{
261- {"10.0.0.1", instance.Ipv4Address, "cloud", instance.NetworkCloudLocal},
262+ []Address{
263+ {"10.0.0.1", Ipv4Address, "cloud", NetworkCloudLocal},
264 },
265 "10.0.0.1",
266 }, {
267 "a machine local address is selected",
268- []instance.Address{
269- {"127.0.0.1", instance.Ipv4Address, "container", instance.NetworkMachineLocal},
270+ []Address{
271+ {"127.0.0.1", Ipv4Address, "container", NetworkMachineLocal},
272 },
273 "127.0.0.1",
274 }}
275@@ -144,6 +147,43 @@
276 func (s *AddressSuite) TestSelectInternalMachineAddress(c *gc.C) {
277 for i, t := range selectInternalMachineTests {
278 c.Logf("test %d. %s", i, t.about)
279- c.Check(instance.SelectInternalAddress(t.addresses, true), gc.Equals, t.expected)
280- }
281+ c.Check(SelectInternalAddress(t.addresses, true), gc.Equals, t.expected)
282+ }
283+}
284+
285+func (*AddressSuite) TestHostAddresses(c *gc.C) {
286+ // Mock the call to net.LookupIP made from HostAddresses.
287+ var lookupIPs []net.IP
288+ var lookupErr error
289+ lookupIP := func(addr string) ([]net.IP, error) {
290+ return append([]net.IP{}, lookupIPs...), lookupErr
291+ }
292+ defer jc.Set(&netLookupIP, lookupIP).Restore()
293+
294+ // err is only non-nil if net.LookupIP fails.
295+ addrs, err := HostAddresses("")
296+ c.Assert(err, gc.IsNil)
297+ // addrs always contains the input address.
298+ c.Assert(addrs, gc.HasLen, 1)
299+ c.Assert(addrs[0], gc.Equals, NewAddress(""))
300+
301+ loopback := net.ParseIP("127.0.0.1").To4()
302+ lookupIPs = []net.IP{net.IPv6loopback, net.IPv4zero, loopback}
303+ addrs, err = HostAddresses("localhost")
304+ c.Assert(err, gc.IsNil)
305+ c.Assert(addrs, gc.HasLen, 4)
306+ c.Assert(addrs[0], gc.Equals, NewAddress(net.IPv6loopback.String()))
307+ c.Assert(addrs[1], gc.Equals, NewAddress(net.IPv4zero.String()))
308+ c.Assert(addrs[2], gc.Equals, NewAddress(loopback.String()))
309+ c.Assert(addrs[3], gc.Equals, NewAddress("localhost"))
310+
311+ lookupErr = errors.New("what happened?")
312+ addrs, err = HostAddresses("localhost")
313+ c.Assert(err, gc.Equals, lookupErr)
314+
315+ // If the input address is an IP, the call to net.LookupIP is elided.
316+ addrs, err = HostAddresses("127.0.0.1")
317+ c.Assert(err, gc.IsNil)
318+ c.Assert(addrs, gc.HasLen, 1)
319+ c.Assert(addrs[0], gc.Equals, NewAddress("127.0.0.1"))
320 }

Subscribers

People subscribed via source and target branches

to status/vote changes: