Various address logic fixes

Various address logic fixes

- Unified logic for Select{Public,Internal}Address.
  Now they both return the first public/private address,
  or the first address with a usable scope.
- instance.NewAddress will now derive the scope of
  IPv4 address from the network range. Loopback addresses
  are machine-local, private network addresses are cloud-
  local, all others are public.
- Now using instance.NewAddress in provider/openstack and
  worker/machiner so scope derivation logic is used.
  Live-tested on HP Cloud and Canonistack.
- Dropped instance.HostAddresses; it is no longer used.
- Fixed state.mergedAddresses to maintain order.

Fixes lp:1303735

https://code.launchpad.net/~axwalk/juju-core/lp1303735-fix-address-logic/+merge/215085

(do not edit description out of merge proposal)

[axw, 2014-04-10 06:54:29]

Please take a look.

[rog, 2014-04-10 07:31:08]

Nice, LGTM.

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

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode34
instance/address.go:34: Ipv4Address AddressType = "ipv4"
It would be great if we could fix the spelling of these to
conform with Go convention (IPv4Address, IPv6Address).
No particular need to do it now though - it's just
been bugging me occasionally when I see it.

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode119
instance/address.go:119: if ip != nil {
if ip == nil {
    return HostName
}

then lose an indent level below?
or even:

switch {
case ip == nil:
    return HostName
case ip.To4() != nil:
    return Ipv4Address
case ip.To6() != nil:
    return Ipv6Address
default:
    panic("Unknown form of IP address"
}

oops, wrote that, then realised it wasn't new code.
feel free to ignore.

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode132
instance/address.go:132: func isIpv4PrivateNetworkAddress(ip net.IP) bool {
s/Ip/IP/

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode170
instance/address.go:170: func NewAddress(value string, scope NetworkScope)
Address {
Perhaps move scope to the first argument here, so that if we add
the same argument to NewAddresses, the two functions can be consistent?

[axw, 2014-04-10 08:13:31]

Please take a look.

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

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode34
instance/address.go:34: Ipv4Address AddressType = "ipv4"
On 2014/04/10 07:31:08, rog wrote:
> It would be great if we could fix the spelling of these to
> conform with Go convention (IPv4Address, IPv6Address).
> No particular need to do it now though - it's just
> been bugging me occasionally when I see it.

Agreed, but not now as discussed.

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode119
instance/address.go:119: if ip != nil {
On 2014/04/10 07:31:08, rog wrote:
> if ip == nil {
>     return HostName
> }
> 
> then lose an indent level below?
> or even:
> 
> switch {
> case ip == nil:
>     return HostName
> case ip.To4() != nil:
>     return Ipv4Address
> case ip.To6() != nil:
>     return Ipv6Address
> default:
>     panic("Unknown form of IP address"
> }
> 
> oops, wrote that, then realised it wasn't new code.
> feel free to ignore.

Easy win, done.

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode132
instance/address.go:132: func isIpv4PrivateNetworkAddress(ip net.IP) bool {
On 2014/04/10 07:31:08, rog wrote:
> s/Ip/IP/

Done.

https://codereview.appspot.com/85590044/diff/1/instance/address.go#newcode170
instance/address.go:170: func NewAddress(value string, scope NetworkScope)
Address {
On 2014/04/10 07:31:08, rog wrote:
> Perhaps move scope to the first argument here, so that if we add
> the same argument to NewAddresses, the two functions can be consistent?

Maybe another time, but this is going to create a lot of noise I'd rather not
have to backport.

[gz, 2014-04-10 11:23:27]

LGTM. Only one real thing to fix, and some other general comments.

https://codereview.appspot.com/85590044/diff/20001/instance/address.go
File instance/address.go (left):

https://codereview.appspot.com/85590044/diff/20001/instance/address.go#oldcod...
instance/address.go:196: mostPublicIndex = i
We're losing a subtle hack here, which is public addresses fallback to the last,
not the first, non-public addr. This is to support the HP style case where
adding a floating ip goes on the end of the list of addresses. Obviously, that
was pretty fragile anyway. With the public/private bits being auto-filled for
openstack anyway, this is probably okay.

https://codereview.appspot.com/85590044/diff/20001/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/85590044/diff/20001/instance/address.go#newcod...
instance/address.go:159: return NetworkPublic
I'm not super-happy with this statement, but I think it will work for all the
cases we care about.

https://codereview.appspot.com/85590044/diff/20001/instance/address_test.go
File instance/address_test.go (right):

https://codereview.appspot.com/85590044/diff/20001/instance/address_test.go#n...
instance/address_test.go:165: "first unknown address selected",
Okay, and here's the test change to go with the logic flip.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider.go:419: NetworkName:  network,
FIX: You've lost the recording of NetworkName - we really do want this when it's
available.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider_test.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider_test.go:63: private:  []nova.IPAddress{{4,
"127.0.0.4"}, {4, "8.8.4.4"}},
And this is the real world example (mostly, not sure why I used a 127. address
not a 10. address) we expect to work... I'm not sure about your changes to this
test? The code should still pick the 8. address if a non-public one is first,
due to the private address range matching?

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider_test.go (right):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider_test.go:82: private:  []nova.IPAddress{{4,
"127.0.0.4"}, {4, "192.168.0.1"}},
Hm, okay, as I'm not actually doing the network name checking any more, making
these 'private' labelled addresses all cloud-local is reasonable.

https://codereview.appspot.com/85590044/diff/20001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/85590044/diff/20001/worker/machiner/machiner.g...
worker/machiner/machiner.go:80: address := instance.NewAddress(ip.String(),
instance.NetworkUnknown)
Good change. We probably still don't want to use this function too widely, as
using machine-reported addresses is a bad idea for most providers. That's a
slight problem with NewAddress actually becoming useful rather than a stub, it
will mostly work in more cases.

[axw, 2014-04-10 11:57:54]

Please take a look.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider.go:419: NetworkName:  network,
On 2014/04/10 11:23:27, gz wrote:
> FIX: You've lost the recording of NetworkName - we really do want this when
it's
> available.

Good catch! Thanks, done.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider_test.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider_test.go:63: private:  []nova.IPAddress{{4,
"127.0.0.4"}, {4, "8.8.4.4"}},
On 2014/04/10 11:23:27, gz wrote:
> And this is the real world example (mostly, not sure why I used a 127. address
> not a 10. address) we expect to work... I'm not sure about your changes to
this
> test? The code should still pick the 8. address if a non-public one is first,
> due to the private address range matching?

I changed it because 127.0.0.4 is machine-local, and hence will never match.
Likewise for the other test changes.

[gz, 2014-04-10 12:20:04]

LGTM.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider_test.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider_test.go:63: private:  []nova.IPAddress{{4,
"127.0.0.4"}, {4, "8.8.4.4"}},
On 2014/04/10 11:57:54, axw wrote:
> On 2014/04/10 11:23:27, gz wrote:
> > And this is the real world example (mostly, not sure why I used a 127.
address
> > not a 10. address) we expect to work... I'm not sure about your changes to
> this
> > test? The code should still pick the 8. address if a non-public one is
first,
> > due to the private address range matching?
> 
> I changed it because 127.0.0.4 is machine-local, and hence will never match.
> Likewise for the other test changes.

So, specifically, the test should be (and should work as) {"10.0.0.1",
"8.8.4.4"} -> "8.8.4.4".

[axw, 2014-04-10 13:22:24]

Please take a look.

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
File provider/openstack/provider_test.go (left):

https://codereview.appspot.com/85590044/diff/20001/provider/openstack/provide...
provider/openstack/provider_test.go:63: private:  []nova.IPAddress{{4,
"127.0.0.4"}, {4, "8.8.4.4"}},
On 2014/04/10 12:20:04, gz wrote:
> On 2014/04/10 11:57:54, axw wrote:
> > On 2014/04/10 11:23:27, gz wrote:
> > > And this is the real world example (mostly, not sure why I used a 127.
> address
> > > not a 10. address) we expect to work... I'm not sure about your changes to
> > this
> > > test? The code should still pick the 8. address if a non-public one is
> first,
> > > due to the private address range matching?
> > 
> > I changed it because 127.0.0.4 is machine-local, and hence will never match.
> > Likewise for the other test changes.
> 
> So, specifically, the test should be (and should work as) {"10.0.0.1",
> "8.8.4.4"} -> "8.8.4.4".

Done.