.include? should return false when passed incompatible IP version objects

[ag-TJNII]

It's a fairly common need to check if one IP is within the range of a subnet, for example:

IPAddress.parse('10.0.0.0/8').include?(IPAddress.parse('10.0.0.1/24'))

In this case the second address will likely be form an external source, and as such might not be the same IP version.
I would expect the following to return false:

IPAddress.parse('10.0.0.0/8').include?(IPAddress.parse('::1'))

because '10.0.0.0/8' definitely does not include ::1. However, it currently throws an exception:

NoMethodError: undefined method `to_u32' for an instance of IPAddress::IPv6 (NoMethodError)

      @prefix <= oth.prefix and network_u32 == (oth.to_u32 & @prefix.to_u32)
                                                   ^^^^^^^
Did you mean?  to_u128
from /home/tom/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv4.rb:588:in `include?'

This bit me today where the test environment was not sufficient to trigger this error. I was expecting false, not an exception.

This was reported in #56, opening as a discussion as requested.

[sandstrom]

I'm somewhat reluctant to support my_ipv4.cover?(my_ipv6), however I agree the error message could be improved with a type-check.

Would that be somewhat helpful?

[ag-TJNII]

I'm curious why you're reluctant to support returning false. As a user, I don't really want to worry about IPv4 vs IPv6 in my code. As both a IPv4 and a IPv6 are IP addresses, in my code I just want to worry about IP addresses. Worrying about the nuance between IPv4 and IPv6 is exactly the kind of problem I want a library to abstract away for me.

I was able to mitigate this pretty quickly with a instance_of? check, but worrying about the instance types of the IPAddress objects I'm passing around feels like something my client code shouldn't need to know about.

[sandstrom]

Good point!

If it wasn't for ipv6-mapped ipv4 addresses, I wouldn't mind.

https://notes.networklessons.com/ipv6-ipv4-mapped-ipv6-address

But for these it feels like either we return false which isn't 100% accurate, or we return true which is also somewhat incorrect (they represent the same address, but cannot always be used interchangeably, due to e.g. lack of support in equipment etc).

Happy to hear your thoughts!

[ag-TJNII]

Yea, that's a fly in the ointment and makes things messy. Such is IPv6...

Reading https://www.rfc-editor.org/rfc/rfc4291.html#section-2.5.5 this mapped block is meant to directly represent the IPv4 space 1:1. Skimming over https://www.rfc-editor.org/rfc/rfc4038 it looks like the intent is to allow for application code to deal with IPv6, and only IPv6, address structures while still being able to communicate with IPv4 networks. So, based on the intent I'm reading in the spec, the IPv4 in IPv6 mapped addresses should be treated the same as IPv4. For example, 198.51.100.48 (Hex: c6.33.64.30) is the same as ::FFFF:C633:6430, it's the same IPv4 address just formatted as IPv6. So I think for the ::FFFF:xxxx:xxxx block returning true is correct and in the spirit of the allocation.

> IPAddress.parse('198.51.100.0/24').include?(IPAddress.parse('198.51.100.48'))
=> true
> IPAddress.parse('198.51.100.0/24').include?(IPAddress.parse('::FFFF:C633:6430'))
[Should also be true]

This opinion doesn't apply to the 6to4 rfc3056 2002:: block as, to the best of my understanding, that's a prefix for NAT not a direct mapping.

Full disclosure: I'm a software developer and Linux sysadmin, not a network engineer. None of the shops I've worked for in the last 15 years have used IPv6, probably because IPv6 is so heavily loaded with complex corner cases like these, so I have little real-world IPv6 experience. We may want to hold this for a bit to see if any network experts chime in.

As a related issue, while making the example above I saw two different exceptions depending on which arguments were IPv4 and IPv6:

The behavior we've been reporting:

> IPAddress.parse('198.51.100.0/24').include?(IPAddress.parse('::FFFF:C633:6430'))
NoMethodError: undefined method `to_u32' for an instance of IPAddress::IPv6 (NoMethodError)

      @prefix <= oth.prefix and network_u32 == (oth.to_u32 & @prefix.to_u32)
                                                   ^^^^^^^
Did you mean?  to_u128
from /home/tom/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv4.rb:588:in `include?'

But If I reverse them:

> IPAddress.parse('::FFFF:C633:6400/24').include?(IPAddress.parse('198.51.100.48'))
ArgumentError: Invalid IP "198.51.100.48" (ArgumentError)

        raise ArgumentError, "Invalid IP #{ip.inspect}"
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
from /home/tom/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv6.rb:101:in `initialize'

As mentioned above I think both these should return true per my interpretation of the spirit of the RFC. However I think the exception currently being different is notable.

[ag-TJNII]

Here's another example that stems from the same core problem:

> IPAddress.parse('198.51.100.48') == IPAddress.parse('::FFFF:C633:6430')
NoMethodError: undefined method `to_u32' for an instance of IPAddress::IPv6 (NoMethodError)

      return prefix <=> oth.prefix if to_u32 == oth.to_u32  
                                                   ^^^^^^^
Did you mean?  to_u128
from /home/tom/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv4.rb:512:in `<=>'

I think this is going to boil down to a user experience decision. Should the gem handle this transparently, or should the user need to take steps to handle it in code?

One thought I did have was perhaps using the IPv4 mapped in IPv6 addresses the other way. In my own use ceases I'm using .parse to convert string inputs into IPAddress objects for actual operations. So perhaps have a .parse method that always returns IPv6, even for IPv4 addresses. For example (and this is a edited/false example):

> IPAddress::IPv6.parse('198.51.100.48')
=> #<IPAddress::IPv6:0x00007f7c9072bd70 @address="0000:0000:0000:0000:0000:ffff:c633:6430", @compressed="::ffff:c633:6430", @groups=[0, 0, 0, 0, 0, 65535, 50739, 25648], @prefix=128>

Then operations can be performed on the IPv6 objects without mixing. This would require documentation to advise users to use IPv6 only in mixed environments, and ideally a standard exception with similar guidance.

[sandstrom]

Good background!

I'm still somewhat on the fence about the my_ipv4.include?(ipv6).

However, (1..2).include?('hello') #=> false so maybe it makes sense (this is somewhat similar, having an integer range and testing if a object of another class is included).

Agree with your 'reversal example' that it's inconsistent how it currently works.

One thought I did have was perhaps using the IPv4 mapped in IPv6 addresses the other way. In my own use ceases I'm using .parse to convert string inputs into IPAddress objects for actual operations. So perhaps have a .parse method that always returns IPv6, even for IPv4 addresses. For example (and this is a edited/false example):

Also an approach, but again I don't want to introduce behavior that's "too magical".