|
|
|
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)
Patch Set 1 #
Total comments: 8
Patch Set 2 : Various address logic fixes #
Total comments: 11
Patch Set 3 : Various address logic fixes #Patch Set 4 : Various address logic fixes #
Total messages: 7
|
axw
Please take a look.
|
11 years, 9 months ago (2014年04月10日 06:54:29 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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?
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.
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.
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.
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".
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.