Unnecessary non-Nil Check in ipallocator.go

[darren]

kubernetes/pkg/registry/core/service/ipallocator/ipallocator.go

Lines 83 to 107 in 6ac6016

	prefix, err := netip.ParsePrefix(cidr.String())
	if err != nil {
	return nil, err
	}
	if prefix.Addr().Is6() && prefix.Bits() < 64 {
	return nil, fmt.Errorf("shortest allowed prefix length for service CIDR is 64, got %d", prefix.Bits())
	}
	// TODO: use the utils/net function once is available
	size := hostsPerNetwork(cidr)
	var family api.IPFamily
	if netutils.IsIPv6CIDR(cidr) {
	family = api.IPv6Protocol
	} else {
	family = api.IPv4Protocol
	}
	// Caching the first, offset and last addresses allows to optimize
	// the search loops by using the netip.Addr iterator instead
	// of having to do conversions with IP addresses.
	// Don't allocate the network's ".0" address.
	ipFirst := prefix.Masked().Addr().Next()
	if err != nil {
	return nil, err
	}

The err is already checked on line 84, so it will always be nil On line 107, the check is redundant

[mauri870]

That is indeed redundant. It seems like someone forgot to remove after a refactor but It was added this way in b022475.

[mauri870]

@darren Feel free to send a pull request by following https://kubernetes.io/docs/contribute/. Thanks.

[darren]

@mauri870

Thanks! I am wondering whether it is necessary to check ip address returned by calling Next() or Prev() is IsValid()

As in https://github.com/golang/go/blob/a85b8810c49515c469d265c399febfa48442a983/src/net/netip/netip.go#L741-L755

But I don't find the overflowed cases yet.

[DrAuYueng]

/sig network

[mauri870]

/assign DrAuYueng

[mauri870]

@mauri870

Thanks! I am wondering whether it is necessary to check ip address returned by calling Next() or Prev() is IsValid()

As in https://github.com/golang/go/blob/a85b8810c49515c469d265c399febfa48442a983/src/net/netip/netip.go#L741-L755

But I don't find the overflowed cases yet.

Hmmm, in theory yes, since .Next() and .Prev() can return an empty netip.Addr. In practice, they probably never overflow in this particular case.

Adding a check for .IsValid() for both Prev and Next would not hurt. I don't expect the path of !ip.IsValid() to be ever taken in these cases, but if they do then that is a bug and it should propagate an error.

[mauri870]

/triage accepted