Update RandIpAddress() so it can use any prefix as input
#305
Conversation
|
|
There was a problem hiding this comment.
Overall I think these changes look good, I have one minor note and there are a couple small lint errors pointed out in the PR.
Also would you mind adding a changelog for this fix?
helper/acctest/random.go
Outdated
| randAddrInt.Add(randAddrInt, big.NewInt(randInt)) | ||
|
|
||
| randAddr, ok := netip.AddrFromSlice(randAddrInt.Bytes()) | ||
| result, _ := netip.AddrFromSlice(resultBytes) |
There was a problem hiding this comment.
nit: I know it's not possible at this point, given the logic above, but I feel like we should keep the original logic of checking the ok return from netip.AddrFromSlice and returning an error with the byte data:
result, ok := netip.AddrFromSlice(resultBytes)
if !ok {
return "", fmt.Errorf("unexpected error creating random address from bytes: %#v", resultBytes)
}There was a problem hiding this comment.
Hey @austinvalle,
I think I took care of your punch list.
Additionally, see the inline comment about the addition of Masked() (a change you didn't request, but seems like a better way to handle slightly-broken input).
Thanks!
| if prefix.Addr().Is4() && prefixSizeExponent > 31 { | ||
| return "", fmt.Errorf("CIDR range is too large: %d", prefixSizeExponent) | ||
| // base address as byte slice | ||
| prefixBytes, err := prefix.Masked().Addr().MarshalBinary() |
There was a problem hiding this comment.
I've added Masked() here to ensure that a user asking for a random address within "192.168.1.77/24" isn't masking out random bits with that "77". They'll get any address within 192.168.1.0-255 this way.
There was a problem hiding this comment.
Ah! That makes sense. I can't really think of a scenario where you'd want to lose that portion of the range, and honestly at that point, the cost of implementing your own random IP function is low 😄
There was a problem hiding this comment.
Thanks for making those changes. Everything new looks good to me 🚀
I have a slight addition to the changelog which I'll resolve and merge. Thanks for the contribution @chrismarget ! This will go out in the upcoming v1.8.0 of the testing module 👍🏻
| if prefix.Addr().Is4() && prefixSizeExponent > 31 { | ||
| return "", fmt.Errorf("CIDR range is too large: %d", prefixSizeExponent) | ||
| // base address as byte slice | ||
| prefixBytes, err := prefix.Masked().Addr().MarshalBinary() |
There was a problem hiding this comment.
Ah! That makes sense. I can't really think of a scenario where you'd want to lose that portion of the range, and honestly at that point, the cost of implementing your own random IP function is low 😄
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
The
RandIpAddress()function has some confusing issues.The prefix length must be less than 31isn't correct. It will accept an IPv4 prefix with a 32-bit mask length, and IPv6 prefix bit masks must be at least 65 bits long. While I have doubts about the utility of "give me a random value from within the range of exactly one host", that functionality is validated by the tests.This PR eliminates the confusing comment and lifts the unnecessary constraints for both protocols so that random addresses can be generated from IPv4 prefixes
0.0.0.0/0through255.255.255.255/32, IPv6 prefixes::/0throughff:ff:ff:ff:ff:ff:ff:ff/128, and anywhere in between.