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

IPy forces the user to provide normalized values #68

Open
SantjagoCorkez opened this issue Feb 21, 2020 · 4 comments
Open

IPy forces the user to provide normalized values #68

SantjagoCorkez opened this issue Feb 21, 2020 · 4 comments

Comments

@SantjagoCorkez
Copy link

Since IPy is a helper library it'd be very good if it does routines itself. Though, you can't provide a non-normalized prefix:

IP('192.156.0.12/24')  # ValueError: IP('192.156.0.12/24') has invalid prefix length (24)

I understand that it's a standard de-facto for prefixes to have their address part be the first address of their range. According to RFC1812:

Internet Address
An assigned number that identifies a host in an internet. It
has two parts: an IP address and a prefix length. The prefix
length indicates how many of the most specific bits of the
address constitute the network

So, if a library's user wants a prefix (by providing a mask along with an IP address to constructor) it would be great if IPy do the Network Address construction by itself, not enforcing a user do the same calculations as IPy does under the hood.

For example, a user has an IP as a string 192.168.99.230

The user wants to check, if another IP 192.168.99.126 belong to the same /25 network. In order to do this a user needs to construct a network address (all 7 last bits dropped to 0) for both (hence, take an integer equivalent) and then compare. By the time a user has ints and a mask, he does not need IPy at all any more.

Instead of raising, IPy could provide a user with functionality like this:

>>> ip1 = IPy.IP('192.168.99.230/25')
>>> ip1
<<< IP('192.168.99.128/25')
>>> ip2 = IPy.IP('192.168.99.126/25')
>>> ip2
<<< IP('192.168.99.0/25')
>>> ip1 == ip2
<<< False
@d33tah
Copy link

d33tah commented Oct 5, 2020

@SantjagoCorkez have you experimented with make_net argument to init?

@SantjagoCorkez
Copy link
Author

@d33tah I've checked the API. No, I didn't try this. Would help. But then I'd propose to change make_net=0 to make_net=False in __init__ declaration to not confuse users.

I'd also propose to make it automatic:

class MakeNetBehaviour(object):
  DONT = 0
  DO = 1
  AUTO = 2


class IPint(object):
  def __init__(self, data, ipversion=0, make_net=MakeNetBehaviour.AUTO):
    ...

and then make it automatic in case the argument value matches AUTO. That is obvious since there's no reasonable situation when one makes a string like 1.2.3.4/24 and he doesn't mean a network.

@bwmetz
Copy link

bwmetz commented Jan 24, 2021

TL;DR @SantjagoCorkez initial report could be considered an unintended bug [see example later] but the fix should not assume the callers intent is to have a network object returned if the IP value provided doesn't fall on the 2-bit boundary that the netmask provided implies as this would create unintended consequences for users of certain use cases. It also should be said that the behavior is in keeping with the Perl Net::IP module that inspired IPy, thus a lot of users of this module likely appreciate that backwards compatibility when migrating legacy Perl programs to Python for the next generation.

First, two observations...

  1. Being able to supply any mask without causing an exception could be useful, e.g. reading/processing a list of system interface addresses
  2. Automatically assuming a network object is the desired output is not, e.g. if altered to support a host IP + netmask, the host IP shouldn't be lost unless specifically requested by the caller to do so

The reason is that no networking device ever considers its interface IP to be x.x.x.x/32, with the exception of loopbacks. The assumption is always local IP and network mask. This seems to imply that the ValueError exception is in fact a bug. Take this IP example from @SantjagoCorkez .

>>> myIP = IP('192.168.99.230/32')
>>> myIP = IP('192.168.99.230/31')
>>> myIP = IP('192.168.99.230/30')
...
ValueError: IP('192.168.99.230/30') has invalid prefix length (30)

This matches the behavior of the Perl's Net::IP.

  DB<1> use Net::IP;
  DB<2> my $ip = new Net::IP('192.168.99.230/32') || warn "Error";
  DB<3> my $ip = new Net::IP('192.168.99.230/31') || warn "Error";
  DB<4> my $ip = new Net::IP('192.168.99.230/30') || warn "Error";
Error at (eval 13)[/usr/share/perl/5.26/perl5db.pl:738] line 2.

With make_net=0 as the default, one would assume that only a /32 can be used without the use of make_net=1, but that is not the case. The output above suggests that you only get an exception when the IP value doesn't fall exactly on a 2-bit boundary represented by the mask, i.e. the a valid network address & mask is actually the expected input. This of course is inconsistent with the make_net=0 default. If make_net=1 were the IPy default, the implied intent would be consistent and prevent an exception but it would change historical operation and likely silently break deployed user code. So if this is a "bug", it is arguably one inherited from Perl's Net::IP due to inspiration, i.e. backwards compatibility.

Therefore I only partially agree with @SantjagoCorkez since one could argue that supplying a specific IP with any network mask should be permitted because the network can always be computed when needed, though it would depart from Perl Net::IP behavior and should be carefully considered with respect to IPy module use to replace legacy Perl implementations. As a helper library, IPy could be made more flexible to accept input like you'd find on an actual network device and perhaps auto-calculate and store the network IP within the object for use by other class functions. One example of this is the original example by @SantjagoCorkez of validating if an IP is on the same network as another, without calculating two network addresses to supply. While make_net=1 can be used to check same network as @d33tah suggested, it only works in simple scenarios.

>>> IP('192.168.99.230/25', make_net=1) == IP('192.168.99.126/25', make_net=1)
False
>>> IP('192.168.99.230/25', make_net=1) == IP('192.168.99.129/25', make_net=1)
True

If testing direct network equality, that would be fine. But what if the network that IP2 is part of falls entirely within the network with IP1? Some would argue that the latter is also a valid "isMemberOf" sort of test. Such a comparison using make_net=1 will report false and that would not be altered by adoption of the first fix suggested by @SantjagoCorkez alone.

>>> IP('192.168.99.230/25', make_net=1) == IP('192.168.99.129/26', make_net=1)
False
>>> IP('192.168.99.230/25', make_net=1)
IP('192.168.99.128/25')
>>> IP('192.168.99.129/26', make_net=1)
IP('192.168.99.128/26')

If the first suggestion on this issue were adopted, perhaps this nested example can be more easily construed as a "isChildOf" sort of test, where the IP and network implied by object IP2 is wholly consumed within the network implied by IP1. Naturally this could be checked with an additional comparison using IP1's netmask and IP2's IPv4 value, to compare parent networks. Unfortunately, there's no easy that I have found to obtain the CIDR netmask from an already existing IP1, short of parsing it as a string once created, but strNetmask() can be used instead.

>>> IP1 = IP('192.168.99.230/25', make_net=1)
>>> IP1
IP('192.168.99.128/25')
>>> IP1.strNormal(1).split('/')[-1]
'25'
>>> IP('192.168.99.129/{}'.format(IP1.strNormal(1).split('/')[-1]), make_net=1)
IP('192.168.99.128/25')
>>> IP1 == IP('192.168.99.129/{}'.format(IP1.strNormal(1).split('/')[-1]), make_net=1)
True

>>> IP1.strNetmask()
'255.255.255.128'
>>> IP('192.168.99.129/{}'.format(IP1.strNetmask()), make_net=1)
IP('192.168.99.128/25')
>>> IP1 == IP('192.168.99.129/{}'.format(IP1.strNetmask()), make_net=1)
True

One might also be capable of doing this sort of test using an IPSet, i.e. by combining the two IPs and checking if the set still contains a single entry or not; however, I haven't found a means to find the # of IP objects within an IPSet. But even this could produce matches you may not intend, i.e. it's better left for creating summary ranges and is excellent at doing that.

>>> IPSet([IP('192.168.99.230/25', make_net=1), IP('192.168.99.126/25', make_net=1)])
IPSet([IP('192.168.99.0/24')])
>>> IPSet([IP('192.168.99.230/25', make_net=1), IP('192.168.99.126/26', make_net=1)])
IPSet([IP('192.168.99.64/26'), IP('192.168.99.128/25')])

>>> IPSet([IP('192.168.99.230/25', make_net=1), IP('192.168.99.129/25', make_net=1)])
IPSet([IP('192.168.99.128/25')])
>>> IPSet([IP('192.168.99.230/25', make_net=1), IP('192.168.99.129/26', make_net=1)])
IPSet([IP('192.168.99.128/25')])

Given the potential impact to existing IPy users, in my opinion this "issue" should be considered very carefully before the code is altered. If continued compatibility with the Perl module that inspired IPy is a key goal, then perhaps adding use cases for the things @SantjagoCorkez wants to accomplish would be better. I'm guessing there are old Perl module examples that could serve as guidance, e.g. see https://metacpan.org/pod/Net::IP#overlaps and https://metacpan.org/pod/Net::IP#ip_is_overlap

@SantjagoCorkez
Copy link
Author

@bwmetz Thanks for your clarification. I appreciate this.

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

3 participants