Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cidr range are a bit messy #534

Closed
martijnhoekstra opened this issue Oct 14, 2023 · 1 comment
Closed

Cidr range are a bit messy #534

martijnhoekstra opened this issue Oct 14, 2023 · 1 comment

Comments

@martijnhoekstra
Copy link
Contributor

damning snippet:

val thirtyinvalid = ipv4"127.168.0.1" / 30
// val thirtyinvalid: com.comcast.ip4s.Cidr[com.comcast.ip4s.Ipv4Address] = 127.168.0.1/30
val thirtyvalid = ipv4"127.168.0.0" / 30
// val thirtyvalid: com.comcast.ip4s.Cidr[com.comcast.ip4s.Ipv4Address] = 127.168.0.0/30
val validIps = (thirtyvalid.prefix.toLong to thirtyvalid.last.toLong).map(Ipv4Address.fromLong)
// val validIps: IndexedSeq[com.comcast.ip4s.Ipv4Address] = Vector(127.168.0.0, 127.168.0.1, 127.168.0.2, 127.168.0.3)
validIps.forall(thirtyinvalid.contains)
// val res1: Boolean = true
validIps.forall(thirtyvalid.contains)
// val res2: Boolean = true
thirtyvalid == thirtyinvalid
// val res3: Boolean = false

This is messy: I would expect two CIDR ranges to be equal iff they represent the same addresses, but that doesn't hold: equality is defined on the base address, but when the base address has bits that are masked, these ranges are different even though they represent the same addresses.

Also note that (ipv4"127.168.0.1" / 30).toString == "127.168.0.1/30" which is not a valid CIDR range.

How to fix it, avoiding breaking compatibility much as possible?

I suggest making sure address == prefix when constructing a CIDR range. This may break the not entirely unreasonable, if buggy, expectation of address == Cidr(address, 16).address to hold for any address, or Cidr(address1, 16) == Cidr(address2, 16) to hold if and only if address1 == address2. This isn't good behaviour, but it's possible people rely on this in the wild.

Another option is to keep taking different addresses, but redefine equality to use prefix rather than address. A big problem of that is that Cidr(address1, 16) == Cidr(address2, 16) would now hold if they represent the same addresses, but now Cidr(address1, 16).address != Cidr(address2, 16).address and maybe even worse, Cidr(address1, 16).productElements != Cidr(address2, 16).productElements

@mpilquist
Copy link
Member

Fixed by #535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants