Fix the bug that GetIndexedIP does not support IPv6

[SataQiu]

Signed-off-by: SataQiu 1527062125@qq.com

Ref: kubernetes/kubernetes#88933

The reason is that the returned byte slice of big.NewInt(0).Add(base, big.NewInt(int64(offset))).Bytes() does not include the leading zero byte.
We need to check and correct the length of the byte slice before converting it to IP.

[SataQiu]

/cc @aojea
/assign @thockin

[aojea]

wow, great catch, is just not the leading zero, it depends on the bigInt representation, per example
using cidr: "00:00:00:1::/120", gives [1 0 0 0 0 0 0 0 0], missing 7 bytes, we have to pad always the results to 16 bytes for ipv6 and 4 byes for ipv4

[aojea]

is this case possible?

[SataQiu]

You can try extreme scenario like this:

ip := net.ParseIP("255.255.255.0")
res := AddNetIPOffset(ip, 256)

Without this line, the underlying byte slice of res will be [1,0,0,0,0]. Not a valid IP.

[aojea]

actually the ip address returned by AddIPOffset() is not valid, is easy to check just adding a println in the code,

	ip := AddIPOffset(BigForIP(subnet.IP), index)
	fmt.Println("ip: ", ip.String())

using subnet.IP: fd:11:b2:be::, we obtain

base:  [253 0 17 0 178 0 190 0 0 0 0 0 0 0 0] <- only 15 
offset:  10
ip:  ?fd001100b200be000000000000000a  <- adds a ? at the beginning 

https://golang.org/src/net/ip.go?s=315:321#L319

@thockin should we modify AddIPOffset() or make that BigForIP() returns the correct size in bytes? right now it seems it removes the leading zeroes answered in next comment

[aojea]

do you know if modifying BigForIP to return the bigInt with correct length would solve the issue?
the benefit is that BigFortIP "knows" the right length and this way we don't have to modify this function passing an additional parameter

[liggitt]

Please don't change existing method signatures. If needed, deprecate this one and delegate to a second method that takes an ipv6 param

[aojea]

modifying BigForIP() is useless, because bigInts are normalized after arithmetics operations, however, we can add a new function that receives an IP as a parameter instead of the bigInt,
I've tested this and it works

// AddNetIPOffset adds the provided integer offset to an IP address
func AddNetIPOffset(ip net.IP, offset int) net.IP {
	length := len(ip)
	base := new(big.Int).SetBytes(ip)
	ipBytes := big.NewInt(0).Add(base, big.NewInt(int64(offset))).Bytes()
	ipBytesLength := len(ipBytes)
	if ipBytesLength < length {
		b := make([]byte, length-ipBytesLength, length)
		return net.IP(append(b, ipBytes...))
	}
	return net.IP(ipBytes)
}

[aojea]

https://golang.org/src/math/big/nat.go

// A number is normalized if the slice contains no leading 0 digits.
// During arithmetic operations, denormalized values may occur but are
// always normalized before returning the final result. The normalized
// representation of 0 is the empty or nil slice (length = 0).

:/

[SataQiu]

fmt.Println(net.ParseIP("00:00:00:be::ff"))
::be:0:0:0:ff

https://play.studygolang.com/p/YqkJBnMv2GP

[aojea]

I guess this is a "tomato, tomato" thing, my understanding is that you can choose where you want to use the ::, it may be at the beginning or at the end of the address, the only restriction is that only can be used once

2. Due to some methods of allocating certain styles of IPv6
   addresses, it will be common for addresses to contain long strings
   of zero bits. In order to make writing addresses containing zero
   bits easier, a special syntax is available to compress the zeros.
   The use of "::" indicates one or more groups of 16 bits of zeros.
   The "::" can only appear once in an address. The "::" can also be
   used to compress leading or trailing zeros in an address.

ref: https://tools.ietf.org/html/rfc4291#page-4

However, the recommendation is, if the blocks of zeroes have the same length, using it for the first sequence of zeroes:

4.2.3. Choice in Placement of "::"
When there is an alternative choice in the placement of a "::", the
longest run of consecutive 16-bit 0 fields MUST be shortened (i.e.,
the sequence with three consecutive zero fields is shortened in 2001:
0:0:1:0:0:0:1). When the length of the consecutive 16-bit 0 fields
are equal (i.e., 2001:db8:0:0:1:0:0:1), the first sequence of zero
bits MUST be shortened. For example, 2001:db8::1:0:0:1 is correct
representation.

ref: https://tools.ietf.org/html/rfc5952#page-10

[SataQiu]

Thanks for your feedback @aojea @liggitt . Updated!

[aojea]

/lgtm
I think we have the implementation right covering all the use cases
/hold
/assign @liggitt
Hold until @liggitt and @thockin define the best way to introduce this change and deprecating AddIPOffset().
Seems that function is not used in k/k directly:

git grep AddIPOffset | grep -v vendor | wc
      0       0       0

but GetIndexedIP is:

git grep GetIndexedIP | grep -v vendor
cmd/kubeadm/app/constants/constants.go: dnsIP, err := utilnet.GetIndexedIP(svcSubnetCIDR, 10)
cmd/kubeadm/app/constants/constants.go: internalAPIServerVirtualIP, err := utilnet.GetIndexedIP(svcSubnet, 1)
cmd/kubeadm/app/preflight/checks.go:    testIP, err := utilsnet.GetIndexedIP(cidr, 1)
cmd/kubeadm/app/util/apiclient/init_dryrun.go:  internalAPIServerVirtualIP, err := utilnet.GetIndexedIP(svcSubnet, 1)
pkg/master/services.go: apiServerServiceIP, err := utilnet.GetIndexedIP(&serviceClusterIPRange, 1)
pkg/registry/core/service/ipallocator/allocator.go:             ip, _ := GetIndexedIP(r.net, offset+1) // +1 because Range doesn't store IP 0
pkg/registry/core/service/ipallocator/allocator.go:// GetIndexedIP returns a net.IP that is subnet.IP + index in the contiguous IP space.
pkg/registry/core/service/ipallocator/allocator.go:func GetIndexedIP(subnet *net.IPNet, index int) (net.IP, error) {

[SataQiu]

kindly ping @thockin !

[SataQiu]

Hi @liggitt
Could you help to review it again? Thx~ 😺

[liggitt]

My comment was only around the mechanics of the k8s.io/utils compatibility. I think someone from sig-network is a better review for the actual change being made.

[thockin]

This PR both fixes the bug and changes the API. Whether you like the API or not, I don't see why it needs to change (or rather, that is orthogonal to the bug)

Why can't we just treat everything at v6, and pad the slice to 16 bytes?

Look at this playground - what test cases would show where this is wrong?

https://play.golang.org/p/IVioicnTFY3

The only really odd thing here is that overflowing a v4 address 255.255.255.255 + 1 just converts into a v6 result. Fixing that would require an API change, which I would be fine to consider in a different PR.

The code, in case playground fails:

package main

import (
	"fmt"
	"math/big"
	"net"
)

func BigForIP(ip net.IP) *big.Int {
	// NOTE: Convert to 16-byte representation so we don't can
	// handle v4 and v6 values the same way.
	return big.NewInt(0).SetBytes(ip.To16())
}

// NOTE: If you started with a v4 address and overflow it, you get a v6 result.
func AddIPOffset(base *big.Int, offset int) net.IP {
	r := big.NewInt(0).Add(base, big.NewInt(int64(offset))).Bytes()
	r = append(make([]byte, 16), r...)
	return net.IP(r[len(r)-16:])
}

func test(in string, off int, exp string) {
	b := BigForIP(net.ParseIP(in))
	res := AddIPOffset(b, off)

	expS := fmt.Sprint([]byte(net.ParseIP(exp)))
	resS := fmt.Sprint([]byte(res))

	if resS != expS {
		fmt.Printf("expected %s, got %s (%v)\n", expS, resS, res)
	}
	if (net.ParseIP(in).To4() == nil) != (res.To4() == nil) {
		fmt.Printf("%s+%d: To4 mismatch: %v %v\n", in, off, net.ParseIP(in).To4() == nil, res.To4() == nil)
	}
	if (net.ParseIP(in).To16() == nil) != (res.To16() == nil) {
		fmt.Printf("%s+%d: To16 mismatch: %v %v\n", in, off, net.ParseIP(in).To16() == nil, res.To16() == nil)
	}
}

func main() {
	test("::0102:0304:0000", 9, "::0102:0304:0009")
	test("00:00:00:1::", 256, "00:00:00:1::0100")
	test("10.0.0.0", 255, "10.0.0.255")
	test("255.255.254.0", 256, "255.255.255.0")
	//test("255.255.255.0", 256, "::1:0:0:0:0") // broken input

}

[danwinship]

That seems like it should work. I don't think the function is really expected to be well-defined in the case where you overflow the IP address.

[alejandrox1]

/kind bug
/priority critical-urgent
/milestone v1.18

[thockin]

@SataQiu Will you have time to adapt this? Otherwise I can take a shot. We'll need to re-vendor it.

[SataQiu]

Thanks for your guidance @thockin !
I will update the PR soon :)

[aojea]

/lgtm
very neat solution
thanks

[aojea]

/hold cancel

[alejandrox1]

Thank you everyone for working on this.
Figure i drop a quick comment for openness and transparency.
The planned release date for Kubernetes 1.18 is next Tuesday (March 24).
Since kubernetes/kubernetes#88933 seems to be related to IPv6 going to beta in 1.18 we (the release team) are considering this work release blocking and will keep on syncing with you all to ensure this gets into 1.18.0.
Right now it seems that the way to help make sure that happens is to push back the release a day or two but I guess right now let's wait and see how things go. Will have a better plan by Monday.
Again, thank you everyone for all the work you do!

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, SataQiu, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment