Just curious, does everyone say /1 is "smaller" than /2? Will it be ambiguous?

That's a good point. Maybe shorter and longer is more appropriate for describing the "length" of the subnet mask. Let me know if you find these descriptions acceptable.

Why can't we use the correct addrs and test addrs==RangeSize(cidr)?

OK. I'll update.

If I understand this, the function is returning the number of hosts that can be expressed by the IPNet. That said, should the message reflect that, instead of in terms of the mask size? Maybe something like "Cannot support more than 2^30 hosts for IPNet 2001::4/64", for example.

In my experience, subnet masks are expressed using "length". I prefer using shorter or longer with a / notation to describe a subnet mask.

Sure. I'm just pointing out (the subtle point) that the method is returning the number of hosts available for the subnet, and wondering if the error message that would get returned, should focus on that, rather than on mask size. For example:

err: = fmt.Sprintf("too many hosts allowed for subnet (%v). Must be /%d or longer", subnet, bits-30)

However, see my last comment on L254, where maybe this should be redesigned to check for valid subnet lengths, rather than returning the number of hosts and having the caller doing some of the checking, with the rest done here. Then, the error messages could be in terms of mask size (from tests for it being too short or too long).

Only concern is that only passing cases are covered.

Agreed. I had fail cases in my original commit but I then removed them. As you mention in your other comment, I think the func needs to be updated to return an error.

Should the function signature be changed to return an error indication? This way, you could unit test for failure cases (I don't know how you can UT panics).

Alternatively, should the function just return the # networks, 0+, and then the caller can check for validity of the number of the range? Right now, it determines the range of hosts AND checks for exceeding a maximum, but not a minimum. Looks like some callers are doing checking.

I'm open to making the changes. Maybe it's best to implement the changes and the failure case UT's in a follow-on PR? Lets see what @MrHohn has to say.

My vote would be to address it now, since the submit queue is blocked. BTW the current assignment has @bowei, instead of MrHohn.

If possible I would change this function to return an error, we should try to avoid panic() in long running server code.

Looking at the usage of this, the function panics (should be returning error, IMHO), if the subnet is too large, and if not, returns the number of hosts available, and the caller then checks if the subnet is too small. Maybe this should be changed to CheckSubnetSize() and return error if too large or too small, and caller can just check return (error) for non-nil?

Looks like it is not too much work to propagate an error instead of panic here. Would be great to fix.

@bowei I removed the panic. @thockin asked to return 0 instead of an error/panic.

is there a reason why this ServiceClusterIPAllocator is Capitalized?

It should not be capitalized. I updated to lowercase.

shouldn't this be return &r, nil?

Thanks for catching this. I have updated the return.

"subnet mask must be >= /2 for IPv4 or /98 for IPv6" (change message to be about valid values)

I have updated the error message.

It should not be capitalized. I updated to lowercase.

Thanks for catching this. I have updated the return.

I have updated the error message.

Put exported error at the top:

ErrMaskTooSmall = error.New("subnet mask must be longer than /1 for IPv4 or /97 for IPv6")

then

return 0, ErrMaskTooSmall

A bit more future proof:

if err != nil {
  if err == ipallocator.ErrMaskTooSmall {
    allErrs = append(allErrs, field.Invalid(fldPath, subnet, "subnet mask is too short"))
  } else {
        allErrs = append(allErrs, field.Invalid(fldPath, subnet, "invalid service subnet"))
  }
}

The name expect isn't clear to me. What does it expect?

I'm not sure what /1 means here - do you mean /31 ? 10.0.0.0/1 is nonsense?

I mean the mask must be /2 or longer (/8, /16, etc.). I did not change any of the existing IPv4 logic in this PR. I updated the error message because it was incorrect.

Old Error:masks greater than 31 bits are not supported

As you can see in the RangeSize function, (bits - ones) >= 31. For IPv4, bits = 32 and ones is the number of masked bits in the 32-bit address. For a /1, the math would trigger the error ``(32 - 1) >= 31`. I am happy to submit a PR to implement a fix if this is not the expected behavior, unless you prefer the fix to be implemented in this PR.

As a future clean-up, we should look at making this easier to understand (or supplement it with a comment to indicate that if the network bits are less than 2 for IPv4 or less than 65 for IPv6 it fails. OK for now.