Skip to content

Commit

Permalink
OCPBUGS-10695: Use subnet mask to detect IP stack
Browse files Browse the repository at this point in the history 
This PR hardens the logic of selecting node IP by adding more
sophisticated way of detecting IPv6 addresses. This is because there is
a class of IPv4-mapped-to-IPv6 addresses which are represented in the
same way as vanilla IPv4 addresses when using net.IP structures.

In order to make our logic smarter, we now analyse subnet mask to decide
if an address is IPv6 or IPv4. This is because we may have an address
`::ffff:192.168.0.160/64` which contains both `:` and `.` making the
regular logic unreliable. Knowing that the biggest mask for IPv4 is /32
we may detect if such an address is an IPv4-mapped-to-IPv6 address or
vanilla IPv4.

To solve the case of IPv4-mapped-to-IPv6 address in a huge v6 subnet
(e.g. `::ffff:192.168.0.160/16`) we are also checking for the capacity
of the struct storing subnet mask.

Contributes-to: OCPBUGS-10695
  • Loading branch information
@mkowalski
mkowalski committed Mar 27, 2023
1 parent 1a0ba6c commit dbe4cc3
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 18 deletions.
36 changes: 21 additions & 15 deletions pkg/utils/addresses.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func addressesRoutingInternal(vips []net.IP, af AddressFilter, getAddrs addressM
}

var routeMap map[int][]netlink.Route
matches := make([]net.IP, 0)
matches := make([]net.IPNet, 0)
for link, addresses := range addrMap {
addrLoop:
for _, address := range addresses {
Expand Down Expand Up @@ -168,9 +168,12 @@ func addressesRoutingInternal(vips []net.IP, af AddressFilter, getAddrs addressM
containmentNet := net.IPNet{IP: address.IP, Mask: route.Dst.Mask}
for _, vip := range vips {
log.Debugf("Checking whether address %s with route %s contains VIP %s", address, route, vip)
if IsNetIPv6(containmentNet) != IsIPv6(vip) {
log.Debugf("Stack of address '%s' and VIP '%s' does not match. Skipping.", address, vip)
}
if containmentNet.Contains(vip) {
log.Debugf("Address %s with route %s contains VIP %s", address, route, vip)
matches = append(matches, address.IP)
matches = append(matches, *address.IPNet)
break addrLoop
}
}
Expand All @@ -179,9 +182,12 @@ func addressesRoutingInternal(vips []net.IP, af AddressFilter, getAddrs addressM
} else {
for _, vip := range vips {
log.Debugf("Checking whether address %s contains VIP %s", address, vip)
if IsNetIPv6(*address.IPNet) != IsIPv6(vip) {
log.Debugf("Stack of address '%s' and VIP '%s' does not match. Skipping.", address, vip)
}
if address.Contains(vip) {
log.Debugf("Address %s contains VIP %s", address, vip)
matches = append(matches, address.IP)
matches = append(matches, *address.IPNet)
break addrLoop
}
}
Expand All @@ -191,8 +197,8 @@ func addressesRoutingInternal(vips []net.IP, af AddressFilter, getAddrs addressM
if len(matches) > 0 {
// Find an address of the opposite IP family on the same interface
for _, address := range addresses {
if IsIPv6(address.IP) != IsIPv6(matches[0]) {
matches = append(matches, address.IP)
if IsNetIPv6(*address.IPNet) != IsNetIPv6(matches[0]) {
matches = append(matches, *address.IPNet)
break
}
}
Expand All @@ -202,9 +208,9 @@ func addressesRoutingInternal(vips []net.IP, af AddressFilter, getAddrs addressM
// Ensure we return addresses in the desired order
sort.SliceStable(matches, func(i, j int) bool {
// Very simplified since we only ever have two items in this list
return IsIPv6(matches[i]) == preferIPv6
return IsNetIPv6(matches[i]) == preferIPv6
})
return matches, nil
return Mapper(matches, func(ipnet net.IPNet) net.IP { return ipnet.IP }), nil
}

// defaultRoute returns true if the passed route is a default route
Expand All @@ -218,7 +224,7 @@ func AddressesDefault(preferIPv6 bool, af AddressFilter) ([]net.IP, error) {
}

type FoundAddress struct {
Address net.IP
Network net.IPNet
Priority int
LinkIndex int
GatewayOnSubnet bool
Expand All @@ -245,7 +251,7 @@ func addressesDefaultInternal(preferIPv6 bool, af AddressFilter, getAddrs addres
log.Debugf("Address %s is on interface %s with default route", address, link.Attrs().Name)
// We should only have one default route per interface
addrs = append(addrs, FoundAddress{
Address: address.IP,
Network: *address.IPNet,
Priority: routeMap[linkIndex][0].Priority,
LinkIndex: linkIndex,
GatewayOnSubnet: address.IPNet.Contains(routeMap[linkIndex][0].Gw),
Expand All @@ -262,13 +268,13 @@ func addressesDefaultInternal(preferIPv6 bool, af AddressFilter, getAddrs addres
sort.SliceStable(addrs, func(i, j int) bool {
if addrs[i].Priority == addrs[j].Priority {
if addrs[i].LinkIndex == addrs[j].LinkIndex {
if IsIPv6(addrs[i].Address) == IsIPv6(addrs[j].Address) {
if IsNetIPv6(addrs[i].Network) == IsNetIPv6(addrs[j].Network) {
if addrs[i].GatewayOnSubnet == addrs[j].GatewayOnSubnet {
return !addrs[i].Address.IsPrivate()
return !addrs[i].Network.IP.IsPrivate()
}
return addrs[i].GatewayOnSubnet
}
return IsIPv6(addrs[i].Address) == preferIPv6
return IsNetIPv6(addrs[i].Network) == preferIPv6
}
return addrs[i].LinkIndex < addrs[j].LinkIndex
}
Expand All @@ -278,11 +284,11 @@ func addressesDefaultInternal(preferIPv6 bool, af AddressFilter, getAddrs addres
foundv4 := false
foundv6 := false
for _, addr := range addrs {
if (IsIPv6(addr.Address) && foundv6) || (!IsIPv6(addr.Address) && foundv4) {
if (IsNetIPv6(addr.Network) && foundv6) || (!IsNetIPv6(addr.Network) && foundv4) {
continue
}
matches = append(matches, addr.Address)
if IsIPv6(addr.Address) {
matches = append(matches, addr.Network.IP)
if IsNetIPv6(addr.Network) {
foundv6 = true
} else {
foundv4 = true
Expand Down
1 change: 1 addition & 0 deletions pkg/utils/addresses_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func addIPv6Addrs(addrs map[netlink.Link][]netlink.Addr, af AddressFilter) {
maybeAddAddress(addrs, af, lo, "::1/128", false)
maybeAddAddress(addrs, af, eth0, "fd00::5/64", false)
maybeAddAddress(addrs, af, eth0, "fe80::1234/64", false)
maybeAddAddress(addrs, af, eth0, "::ffff:192.168.1.160/64", false)
maybeAddAddress(addrs, af, eth1, "fd69::2/125", false)
maybeAddAddress(addrs, af, eth1, "fd01::3/64", true)
maybeAddAddress(addrs, af, eth1, "fd01::4/64", true)
Expand Down
28 changes: 27 additions & 1 deletion pkg/utils/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,40 @@ func SplitCIDR(s string) (string, string, error) {
}

func IsIPv4(ip net.IP) bool {
// func IsIPv4(ip string) bool {
return strings.Contains(ip.String(), ".")
}

func IsIPv6(ip net.IP) bool {
return strings.Contains(ip.String(), ":")
}

func IsNetIPv6(network net.IPNet) bool {
if IsIPv6(network.IP) {
return true
}

if IsIPv4(network.IP) {
// Sadly the fact that address has "." doesn't mean it's an IPv4 address. We need to check
// subnet masks to make sure this is really the case. If subnet mask is longer than 32 we
// are for sure dealing with IPv6 address. If it is shorter, then we need to look at the
// total capacity of the mask as it can be simply an IPv4 address but could be as well an
// IPv6 in really enormous subnet.
//
// To simplify this logic because subnet longer than 32 can happen if the capacity of mask
// matches IPv6, we can simply look at the capacity of the mask and determine IP stack.
_, n := network.Mask.Size()
if n == 32 {
return false
}
if n == 128 {
return true
}
}

log.Debugf("Failed to find IP stack of '%+v'", &network)
return false
}

func IpInCidr(ipAddr, cidr string) (bool, error) {
_, ipNet, err := net.ParseCIDR(cidr)
if err != nil {
Expand Down
87 changes: 87 additions & 0 deletions pkg/utils/network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,93 @@ var _ = Describe("IP stack detection", func() {
res := IsIPv6(net.ParseIP("this-is-not-an-ip"))
Expect(res).To(Equal(false))
})

})

Context("for IPv4-mapped-to-IPv6 addresses", func() {
// (mko) This test suite is intentionally returning wrong results. This is because the
// mapped form of those addresses is making them impossible to distinguish without
// knowing more properties of the link.
//
// Due to the fact how net.IPv4() initialization is implemented and the fact that it
// always adds the v4InV6Prefix header in a binary form, those addresses look like IPv4
// and in most cases behave like such.
//
// The tests here are saying that "::ffff:192.168.0.14" is IPv4 and not an IPv6 address
// whenever fed into our own IsIPv4 and IsIPv6 functions. For a proper answer returning
// IPv6 for those, we have IsNetIPv6 function which looks at IP address as well as subnet
// mask.

It("IsIPv4 returns true for IPv4-mapped-to-IPv6 address", func() {
res := IsIPv4(net.ParseIP("::ffff:192.168.0.14"))
Expect(res).To(Equal(true))
})
It("IsIPv4 returns true for binary IPv4-mapped-to-IPv6 address", func() {
ip := net.IP([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 192, 168, 1, 160})
res := IsIPv4(ip)
Expect(res).To(Equal(true))
})

It("IsIPv6 returns false for IPv4-mapped-to-IPv6 address", func() {
res := IsIPv6(net.ParseIP("::ffff:192.168.0.14"))
Expect(res).To(Equal(false))
})
It("IsIPv6 returns true for binary IPv4-mapped-to-IPv6 address", func() {
ip := net.IP([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 192, 168, 1, 160})
res := IsIPv6(ip)
Expect(res).To(Equal(false))
})
})

Context("using IsNetIPv6", func() {
It("returns false for IPv4 addresses", func() {
addrs := []string{
"192.168.0.1/24",
"192.168.0.1/32",
"192.168.0.1/0",
"0.0.0.0/0",
}
for _, addr := range addrs {
_, addr, _ := net.ParseCIDR(addr)
res := IsNetIPv6(*addr)
Expect(res).To(Equal(false))
}
})
It("returns true for IPv6 address", func() {
addrs := []string{
"fe80::84ca:4a24:ffbf:1778/64",
"::/0",
"::1/128",
"::ffff:192.168.0.14/64",
"::ffff:192.168.0.14/16",
}
for _, addr := range addrs {
_, addr, _ := net.ParseCIDR(addr)
res := IsNetIPv6(*addr)
Expect(res).To(Equal(true))
}
})
It("returns true for IPv4-mapped-to-IPv6 address", func() {
// Because of non-obvious behaviour of net library when it comes to
// IPv4-mapped-to-IPv6 addresses (more in https://github.com/golang/go/issues/51906)
// we are adding tests where we explicitly create net.IPNet struct from a binary
// input.
addrs := []net.IPNet{
{
IP: net.IP([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 192, 168, 1, 160}),
Mask: net.IPMask([]byte{255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0}),
},
{
IP: net.IP([]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 192, 168, 1, 160}),
Mask: net.IPMask([]byte{255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}),
},
}

for _, addr := range addrs {
res := IsNetIPv6(addr)
Expect(res).To(Equal(true))
}
})
})
})

Expand Down
13 changes: 11 additions & 2 deletions pkg/utils/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ import (
"fmt"
"io"
"io/ioutil"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"net/http"
"os"
"strings"
"time"

"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"

"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -135,3 +136,11 @@ func GetClientConfig(kubeApiServerUrl, kubeconfigPath string) (*rest.Config, err
config.Timeout = kubeClientTimeout
return config, err
}

func Mapper[T, U any](data []T, f func(T) U) []U {
res := make([]U, 0, len(data))
for _, e := range data {
res = append(res, f(e))
}
return res
}

0 comments on commit dbe4cc3

Please sign in to comment.