Merge lp:~axwalk/juju-core/lp1300264-manual-unresolvable-address into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/manual: don't require resolvable hostname

This change makes it so that the hostname provided
during manual provisioning is not necessarily
resolvable from the machine provisioning. If this
is the case, the machine will not be given a public
address.

There's another change here, which is that we no
longer perform a reverse lookup or record the
resolved addresses in state; we only store the
input address if it's an IP or if it's a resolvable
hostname. All other addresses will come from the
machine agent itself.

Fixes lp:1300264

https://codereview.appspot.com/82930044/

environs/manual: don't require resolvable hostname

This change makes it so that the hostname provided
during manual provisioning is not necessarily
resolvable from the machine provisioning. If this
is the case, the machine will not be given a public
address.

There's another change here, which is that we no
longer perform a reverse lookup or record the
resolved addresses in state; we only store the
input address if it's an IP or if it's a resolvable
hostname. All other addresses will come from the
machine agent itself.

Fixes lp:1300264

https://codereview.appspot.com/82930044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+213581_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: don't require resolvable hostname

This change makes it so that the hostname provided
during manual provisioning is not necessarily
resolvable from the machine provisioning. If this
is the case, the machine will not be given a public
address.

There's another change here, which is that we no
longer perform a reverse lookup or record the
resolved addresses in state; we only store the
input address if it's an IP or if it's a resolvable
hostname. All other addresses will come from the
machine agent itself.

Fixes lp:1300264

https://code.launchpad.net/~axwalk/juju-core/lp1300264-manual-unresolvable-address/+merge/213581

(do not edit description out of merge proposal)

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

Affected files (+86, -60 lines):
   A [revision details]
   M environs/manual/addresses.go
   M environs/manual/addresses_test.go
   M environs/manual/export_test.go
   M environs/manual/provisioner.go
   M provider/manual/instance.go

[Ian Booth (wallyworld)]

LGTM but please consider the suggestion to improve the test

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

https://codereview.appspot.com/82930044/diff/1/environs/manual/addresses_test.go#newcode22
environs/manual/addresses_test.go:22: func (s *addressesSuite)
TestHostAddress(c *gc.C) {
I'd prefer this test split up into different tests for IP address vs
hostname etc. Also, it would be good if the mocked NetLookupHost
function would error based on the input arg eg if host were "valid.host"
it would resolve, if it were "invalid.host", it would raise an error. I
think that's cleaner than the approach taken and removes the extra
variables.

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

https://codereview.appspot.com/82930044/diff/1/environs/manual/provisioner.go#newcode236
environs/manual/provisioner.go:236: addrs = append(addrs, addr)
Did we want any logging here?

https://codereview.appspot.com/82930044/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/82930044/diff/1/environs/manual/addresses_test.go#newcode22
environs/manual/addresses_test.go:22: func (s *addressesSuite)
TestHostAddress(c *gc.C) {
On 2014/04/01 04:46:17, wallyworld wrote:
> I'd prefer this test split up into different tests for IP address vs
hostname
> etc. Also, it would be good if the mocked NetLookupHost function would
error
> based on the input arg eg if host were "valid.host" it would resolve,
if it were
> "invalid.host", it would raise an error. I think that's cleaner than
the
> approach taken and removes the extra variables.

Done.

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

https://codereview.appspot.com/82930044/diff/1/environs/manual/provisioner.go#newcode236
environs/manual/provisioner.go:236: addrs = append(addrs, addr)
On 2014/04/01 04:46:17, wallyworld wrote:
> Did we want any logging here?

There's nothing really to log. We no longer use the result of resolving,
other than success/failure. No log message == success.

https://codereview.appspot.com/82930044/

=== modified file 'environs/manual/addresses.go'
--- environs/manual/addresses.go	2014-01-30 03:18:20 +0000
+++ environs/manual/addresses.go	2014-04-02 02:23:48 +0000
@@ -4,20 +4,33 @@
 package manual
 
 import (
+	"net"
+
 	"launchpad.net/juju-core/instance"
 )
 
-var instanceHostAddresses = instance.HostAddresses
+var netLookupHost = net.LookupHost
 
-// HostAddresses returns the addresses for the specified
-// hostname, and marks the input address as being public;
-// all other addresses have "unknown" scope.
-func HostAddresses(hostname string) ([]instance.Address, error) {
-	addrs, err := instanceHostAddresses(hostname)
-	if err != nil {
-		return nil, err
-	}
-	// The final address is the one we fed in: mark it as public.
-	addrs[len(addrs)-1].NetworkScope = instance.NetworkPublic
-	return addrs, nil
+// HostAddress returns an instance.Address for the specified
+// hostname, depending on whether it is an IP or a resolvable
+// hostname. The address is given public scope.
+func HostAddress(hostname string) (instance.Address, error) {
+	if ip := net.ParseIP(hostname); ip != nil {
+		addr := instance.Address{
+			Value:        ip.String(),
+			Type:         instance.DeriveAddressType(ip.String()),
+			NetworkScope: instance.NetworkPublic,
+		}
+		return addr, nil
+	}
+	// Only a resolvable hostname may be used as a public address.
+	if _, err := netLookupHost(hostname); err != nil {
+		return instance.Address{}, err
+	}
+	addr := instance.Address{
+		Value:        hostname,
+		Type:         instance.HostName,
+		NetworkScope: instance.NetworkPublic,
+	}
+	return addr, nil
 }

=== modified file 'environs/manual/addresses_test.go'
--- environs/manual/addresses_test.go	2014-03-28 06:56:55 +0000
+++ environs/manual/addresses_test.go	2014-04-02 02:23:48 +0000
@@ -4,6 +4,8 @@
 package manual_test
 
 import (
+	"errors"
+
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/environs/manual"
@@ -11,27 +13,65 @@
 	"launchpad.net/juju-core/testing/testbase"
 )
 
+const (
+	invalidHost = "testing.invalid"
+	validHost   = "testing.valid"
+)
+
 type addressesSuite struct {
 	testbase.LoggingSuite
+	netLookupHostCalled int
 }
 
 var _ = gc.Suite(&addressesSuite{})
 
-func (s *addressesSuite) TestHostAddresses(c *gc.C) {
-	const hostname = "boxen0"
-	s.PatchValue(manual.InstanceHostAddresses, func(host string) ([]instance.Address, error) {
-		c.Check(host, gc.Equals, hostname)
-		return []instance.Address{
-			instance.NewAddress("192.168.0.1", instance.NetworkUnknown),
-			instance.NewAddress("nickname", instance.NetworkUnknown),
-			instance.NewAddress(hostname, instance.NetworkUnknown),
-		}, nil
-	})
-	addrs, err := manual.HostAddresses(hostname)
-	c.Assert(err, gc.IsNil)
-	c.Assert(addrs, gc.HasLen, 3)
-	// The last address is marked public, all others are unknown.
-	c.Assert(addrs[0].NetworkScope, gc.Equals, instance.NetworkUnknown)
-	c.Assert(addrs[1].NetworkScope, gc.Equals, instance.NetworkUnknown)
-	c.Assert(addrs[2].NetworkScope, gc.Equals, instance.NetworkPublic)
+func (s *addressesSuite) SetUpTest(c *gc.C) {
+	s.netLookupHostCalled = 0
+	s.PatchValue(manual.NetLookupHost, func(host string) ([]string, error) {
+		s.netLookupHostCalled++
+		if host == invalidHost {
+			return nil, errors.New("invalid host: " + invalidHost)
+		}
+		return []string{"127.0.0.1"}, nil
+	})
+}
+
+func (s *addressesSuite) TestHostAddress(c *gc.C) {
+	addr, err := manual.HostAddress(validHost)
+	c.Assert(s.netLookupHostCalled, gc.Equals, 1)
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, instance.Address{
+		Value:        validHost,
+		Type:         instance.HostName,
+		NetworkScope: instance.NetworkPublic,
+	})
+}
+
+func (s *addressesSuite) TestHostAddressError(c *gc.C) {
+	addr, err := manual.HostAddress(invalidHost)
+	c.Assert(s.netLookupHostCalled, gc.Equals, 1)
+	c.Assert(err, gc.ErrorMatches, "invalid host: "+invalidHost)
+	c.Assert(addr, gc.Equals, instance.Address{})
+}
+
+func (s *addressesSuite) TestHostAddressIPv4(c *gc.C) {
+	addr, err := manual.HostAddress("127.0.0.1")
+	c.Assert(s.netLookupHostCalled, gc.Equals, 0)
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, instance.Address{
+		Value:        "127.0.0.1",
+		Type:         instance.Ipv4Address,
+		NetworkScope: instance.NetworkPublic,
+	})
+}
+
+func (s *addressesSuite) TestHostAddressIPv6(c *gc.C) {
+	addr, err := manual.HostAddress("::1")
+	c.Assert(s.netLookupHostCalled, gc.Equals, 0)
+	c.Assert(err, gc.IsNil)
+	c.Assert(addr, gc.Equals, instance.Address{
+		Value:        "::1",
+		Type:         instance.Ipv6Address,
+		NetworkScope: instance.NetworkPublic,
+	})
 }

=== modified file 'environs/manual/export_test.go'
--- environs/manual/export_test.go	2014-03-12 12:21:40 +0000
+++ environs/manual/export_test.go	2014-04-02 02:23:48 +0000
@@ -4,7 +4,7 @@
 package manual
 
 var (
-	InstanceHostAddresses = &instanceHostAddresses
+	NetLookupHost         = &netLookupHost
 	ProvisionMachineAgent = &provisionMachineAgent
 	CheckProvisioned      = checkProvisioned
 )

=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go	2014-03-17 03:17:04 +0000
+++ environs/manual/provisioner.go	2014-04-02 02:23:48 +0000
@@ -8,7 +8,6 @@
 	"errors"
 	"fmt"
 	"io"
-	"net"
 	"strings"
 
 	"github.com/juju/loggo"
@@ -229,34 +228,13 @@
 	if err != nil {
 		return nil, err
 	}
-	// First, gather the parameters needed to inject the existing host into state.
-	if ip := net.ParseIP(hostname); 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.Infof("failed to resolve %v: %v", ip, err)
-		} else {
-			logger.Infof("resolved %v to %v", ip, names)
-			hostname = names[0]
-			// TODO: jam 2014-01-09 https://bugs.launchpad.net/bugs/1267387
-			// We change what 'hostname' we are using here (rather
-			// than an IP address we use the DNS name). I'm not
-			// sure why that is better, but if we are changing the
-			// host, we should probably be returning the hostname
-			// to the parent function.
-			// Also, we don't seem to try and compare if 'ip' is in
-			// the list of addrs returned from
-			// instance.HostAddresses in case you might get
-			// multiple and one of them is what you are supposed to
-			// be using.
-		}
-	}
-	addrs, err := HostAddresses(hostname)
-	if err != nil {
-		return nil, err
-	}
-	logger.Infof("addresses for %v: %v", hostname, addrs)
+
+	var addrs []instance.Address
+	if addr, err := HostAddress(hostname); err != nil {
+		logger.Warningf("failed to compute public address for %q: %v", hostname, err)
+	} else {
+		addrs = append(addrs, addr)
+	}
 
 	provisioned, err := checkProvisioned(hostname)
 	if err != nil {

=== modified file 'provider/manual/instance.go'
--- provider/manual/instance.go	2014-01-31 05:38:42 +0000
+++ provider/manual/instance.go	2014-04-02 02:23:48 +0000
@@ -26,7 +26,11 @@
 }
 
 func (inst manualBootstrapInstance) Addresses() (addresses []instance.Address, err error) {
-	return manual.HostAddresses(inst.host)
+	addr, err := manual.HostAddress(inst.host)
+	if err != nil {
+		return nil, err
+	}
+	return []instance.Address{addr}, nil
 }
 
 func (inst manualBootstrapInstance) DNSName() (string, error) {