net: fix IPv4 address selection

Only apply RFC 6724's CommonPrefixLen rule for IPv4 source/destination
pairs that are members of the same IPv4 special purpose block.

Fixes #13283.

Change-Id: I2f7c26b408dd4675dfc5c1959e22d05b43bb8241
Reviewed-on: https://go-review.googlesource.com/16995
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/addrselect.go b/src/net/addrselect.go
index e22fbac5cee1a99ac0ae7fa7ff31e6e4036a15a0..58ab7d706c61be3db190b14f21277e79437ba23a 100644 (file)
--- a/src/net/addrselect.go
+++ b/src/net/addrselect.go
@@ -197,6 +197,24 @@ func (s *byRFC6724) Less(i, j int) bool {
        if da4 == db4 {
                commonA := commonPrefixLen(SourceDA, DA)
                commonB := commonPrefixLen(SourceDB, DB)
+
+               // CommonPrefixLen doesn't really make sense for IPv4, and even
+               // causes problems for common load balancing practices
+               // (e.g., https://golang.org/issue/13283).  Glibc instead only
+               // uses CommonPrefixLen for IPv4 when the source and destination
+               // addresses are on the same subnet, but that requires extra
+               // work to find the netmask for our source addresses.  As a
+               // simpler heuristic, we limit its use to when the source and
+               // destination belong to the same special purpose block.
+               if da4 {
+                       if !sameIPv4SpecialPurposeBlock(SourceDA, DA) {
+                               commonA = 0
+                       }
+                       if !sameIPv4SpecialPurposeBlock(SourceDB, DB) {
+                               commonB = 0
+                       }
+               }
+
                if commonA > commonB {
                        return preferDA
                }
@@ -386,3 +404,28 @@ func commonPrefixLen(a, b IP) (cpl int) {
        }
        return
 }
+
+// sameIPv4SpecialPurposeBlock reports whether a and b belong to the same
+// address block reserved by the IANA IPv4 Special-Purpose Address Registry:
+// http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
+func sameIPv4SpecialPurposeBlock(a, b IP) bool {
+       a, b = a.To4(), b.To4()
+       if a == nil || b == nil || a[0] != b[0] {
+               return false
+       }
+       // IANA defines more special-purpose blocks, but these are the only
+       // ones likely to be relevant to typical Go systems.
+       switch a[0] {
+       case 10: // 10.0.0.0/8: Private-Use
+               return true
+       case 127: // 127.0.0.0/8: Loopback
+               return true
+       case 169: // 169.254.0.0/16: Link Local
+               return a[1] == 254 && b[1] == 254
+       case 172: // 172.16.0.0/12: Private-Use
+               return a[1]&0xf0 == 16 && b[1]&0xf0 == 16
+       case 192: // 192.168.0.0/16: Private-Use
+               return a[1] == 168 && b[1] == 168
+       }
+       return false
+}

diff --git a/src/net/addrselect_test.go b/src/net/addrselect_test.go
index 562022772fa853c0e543d07b2502c91a117f5f1a..80aa4eb195b535ffb1811b2c60660221def9429c 100644 (file)
--- a/src/net/addrselect_test.go
+++ b/src/net/addrselect_test.go
@@ -87,6 +87,57 @@ func TestSortByRFC6724(t *testing.T) {
                        },
                        reverse: true,
                },
+
+               // Issue 13283.  Having a 10/8 source address does not
+               // mean we should prefer 23/8 destination addresses.
+               {
+                       in: []IPAddr{
+                               {IP: ParseIP("54.83.193.112")},
+                               {IP: ParseIP("184.72.238.214")},
+                               {IP: ParseIP("23.23.172.185")},
+                               {IP: ParseIP("75.101.148.21")},
+                               {IP: ParseIP("23.23.134.56")},
+                               {IP: ParseIP("23.21.50.150")},
+                       },
+                       srcs: []IP{
+                               ParseIP("10.2.3.4"),
+                               ParseIP("10.2.3.4"),
+                               ParseIP("10.2.3.4"),
+                               ParseIP("10.2.3.4"),
+                               ParseIP("10.2.3.4"),
+                               ParseIP("10.2.3.4"),
+                       },
+                       want: []IPAddr{
+                               {IP: ParseIP("54.83.193.112")},
+                               {IP: ParseIP("184.72.238.214")},
+                               {IP: ParseIP("23.23.172.185")},
+                               {IP: ParseIP("75.101.148.21")},
+                               {IP: ParseIP("23.23.134.56")},
+                               {IP: ParseIP("23.21.50.150")},
+                       },
+                       reverse: false,
+               },
+
+               // Prefer longer common prefixes, but only for IPv4 address
+               // pairs in the same special-purpose block.
+               {
+                       in: []IPAddr{
+                               {IP: ParseIP("1.2.3.4")},
+                               {IP: ParseIP("10.55.0.1")},
+                               {IP: ParseIP("10.66.0.1")},
+                       },
+                       srcs: []IP{
+                               ParseIP("1.2.3.5"),
+                               ParseIP("10.66.1.2"),
+                               ParseIP("10.66.1.2"),
+                       },
+                       want: []IPAddr{
+                               {IP: ParseIP("10.66.0.1")},
+                               {IP: ParseIP("10.55.0.1")},
+                               {IP: ParseIP("1.2.3.4")},
+                       },
+                       reverse: true,
+               },
        }
        for i, tt := range tests {
                inCopy := make([]IPAddr, len(tt.in))
@@ -217,3 +268,67 @@ func TestRFC6724CommonPrefixLength(t *testing.T) {
        }
 
 }
+
+func mustParseCIDRs(t *testing.T, blocks ...string) []*IPNet {
+       res := make([]*IPNet, len(blocks))
+       for i, block := range blocks {
+               var err error
+               _, res[i], err = ParseCIDR(block)
+               if err != nil {
+                       t.Fatalf("ParseCIDR(%s) failed: %v", block, err)
+               }
+       }
+       return res
+}
+
+func TestSameIPv4SpecialPurposeBlock(t *testing.T) {
+       blocks := mustParseCIDRs(t,
+               "10.0.0.0/8",
+               "127.0.0.0/8",
+               "169.254.0.0/16",
+               "172.16.0.0/12",
+               "192.168.0.0/16",
+       )
+
+       addrs := []struct {
+               ip    IP
+               block int // index or -1
+       }{
+               {IP{1, 2, 3, 4}, -1},
+               {IP{2, 3, 4, 5}, -1},
+               {IP{10, 2, 3, 4}, 0},
+               {IP{10, 6, 7, 8}, 0},
+               {IP{127, 0, 0, 1}, 1},
+               {IP{127, 255, 255, 255}, 1},
+               {IP{169, 254, 77, 99}, 2},
+               {IP{169, 254, 44, 22}, 2},
+               {IP{169, 255, 0, 1}, -1},
+               {IP{172, 15, 5, 6}, -1},
+               {IP{172, 16, 32, 41}, 3},
+               {IP{172, 31, 128, 9}, 3},
+               {IP{172, 32, 88, 100}, -1},
+               {IP{192, 168, 1, 1}, 4},
+               {IP{192, 168, 128, 42}, 4},
+               {IP{192, 169, 1, 1}, -1},
+       }
+
+       for i, addr := range addrs {
+               for j, block := range blocks {
+                       got := block.Contains(addr.ip)
+                       want := addr.block == j
+                       if got != want {
+                               t.Errorf("%d/%d. %s.Contains(%s): got %v, want %v", i, j, block, addr.ip, got, want)
+                       }
+               }
+       }
+
+       for i, addr1 := range addrs {
+               for j, addr2 := range addrs {
+                       got := sameIPv4SpecialPurposeBlock(addr1.ip, addr2.ip)
+                       want := addr1.block >= 0 && addr1.block == addr2.block
+                       if got != want {
+                               t.Errorf("%d/%d. sameIPv4SpecialPurposeBlock(%s, %s): got %v, want %v", i, j, addr1.ip, addr2.ip, got, want)
+                       }
+               }
+       }
+}