classification
Title: containment test for "ip_network in ip_network"
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pmoody Nosy List: James Schneider, JamesGuthrie, Mark.Ignacio, SilentGhost, berker.peksag, cheryl.sabella, eric.smith, exhuma, gescheit, ncoghlan, pitrou, pmoody, r.david.murray
Priority: normal Keywords: patch

Created on 2014-03-02 13:18 by exhuma, last changed 2017-10-22 21:40 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
net-in-net.patch exhuma, 2014-03-02 13:18 review
net-in-net-r2.patch exhuma, 2014-03-06 11:50 review
net-in-net-r3.patch exhuma, 2014-03-23 15:31 review
net-in-net-r4.patch exhuma, 2016-06-25 10:47 review
net-in-net-r5.patch exhuma, 2016-06-25 11:33 review
net-in-net-r6.patch exhuma, 2016-06-25 14:35 review
Pull Requests
URL Status Linked Edit
PR 4065 merged cheryl.sabella, 2017-10-20 23:53
Messages (30)
msg212550 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-02 13:18
The ipaddress module always returns ``False`` when testing if a network is contained in another network. However, I feel this should be a valid test. No? Is there any reason why this is fixed to ``False``?

In case not, here's a patch which implements this test.

Note that by design, IP addresses networks can never overlap "half-way". In cases where this should return ``False``, you either have a network that lies completely "to the left", or completely "to the right". In the case it should return ``True`` the smaller network is always completely bounded by the larger network's network- and broadcast address.

I needed to change two containment tests as they were in conflict with this change. These tests were ``self.v6net not in self.v6net`` and ``self.v4net not in self.v4net``. The reason these two failed is that the new containment test checks the target range *including* broadcast and network address. So ``a in a`` always returns true.

This could be changed by excluding one of the two boundaries, and by that forcing the "containee" to be smaller than the "container". But it would make the check a bit more complex, as you would need to add an exception for the case that both are identical.

Backwards compatibility is a good question. Strictly put, this would break it. However, I can't think of any reason why anyone would expect ``a in a`` to be false in the case of IP-Addresses.

Just as a side-note, I currently work at our national network provider and am currently implementing a tool dealing with a lot of IP-Addresses. We have run into the need to test ``net in net`` a couple of times and ran into bugs because the stdlib returns ``False`` where you technically expect it to be ``True``.
msg212551 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-02 13:56
I don't think "in" is the right operator for this. You can draw an analogy with sets:

>>> a = {1,2}
>>> b = {1,2,3}
>>> a <= b
True
>>> a in b
False

In mathematical terms, there is a difference between inclusion (being a subset of) and containment (being an element of).
msg212552 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-02 14:07
Hmm... after thinking about this, I kind of agree. I was about to state something about the fact that you could consider networks like an "ordered set". And use that to justify my addition :) But the more I think about it, the more I am okay with your point.

I quickly tested the following:

    >>> a = ip_network('10.0.0.0/24')
    >>> b = ip_network('10.0.0.0/30')
    >>> a <= b
    True
    >>> b <= a
    False

Which is wrong when considering "containement".

What about an instance-method? Something like ``b.contained_in(a)``?

At least that would be explicit and avoids confusion. Because the existing ``__lt__`` implementation makes sense in the way it's already used.
msg212558 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-02 15:15
It seems from the ipaddress documentation that 'a.contains(b)' would appropriate.  But to avoid confusion with 'in', you could say a.covers(b).

However, 'in' is already used for <address> in <network>, and you can already get subnets out of a network (via 'subnets'), so it isn't completely crazy to consider the network a container of subnets (of varying sizes, depending on the arguments to subnet).
msg212560 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-02 15:23
> However, 'in' is already used for <address> in <network>, and you can
> already get subnets out of a network (via 'subnets'), so it isn't
> completely crazy to consider the network a container of subnets (of
> varying sizes, depending on the arguments to subnet).

So how about subnet_of(other)?
(and the reciprocal supernet_of(other))
msg212695 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-04 08:19
I second "supernet_of" and "subnet_of". I'll implement it as soon as I get around it.

I have been thinking about using ``in`` and ``<=`` and, while I initially liked the idea for tests, I find both operators too ambiguous.

With ``in`` there's the already mentioned ambiguity of containment/inclusion. And ``<=`` could mean is a smaller size (has less individual hosts), but could also mean that it is a subnet, or even that it is "located to the left".

Naming it ``subnet_of`` makes it 100% clear what it does.

Currently, ``a <= b`` returns ``True`` if a comes before/lies on the left of b.
msg212792 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2014-03-06 03:10
There is some history for using "in" for containment. I'm porting some code from IPy (https://pypi.python.org/pypi/IPy/), and it uses "in".

It would make my life easier if "in" worked in ipaddress, but then again it would have to be a previously release version of ipaddress. So I'm open to any names. It's definitely a useful feature.
msg212805 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-06 11:50
Here's a new patch implementing both ``subnet_of`` and ``supernet_of``.

It also contains the relevant docs and unit-tests.
msg213167 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2014-03-11 19:31
subnet_of and supernet_of also avoid confusion with the IP?Interface classes (which incidentally can be used in 'a in b' containment tests).

Michael, have you signed the contributor license agreement? I don't think I have anyway of seeing if you have or not and I think you need to sign it for me to apply you patch.

http://www.python.org/psf/contrib/contrib-form/
msg213169 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-11 19:38
Yes. I signed it last Friday if I recall correctly.

As I understood it, the way for you to tell if it's done, is that my username will be followed by an asterisk.

But I'm not in a hurry. Once I get the confirmation, I can just ping you again via a comment here, so you don't need to monitor it yourself.
msg213173 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2014-03-11 20:22
Thanks!
msg214561 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-23 10:54
Hi again,

The contribution agreement has been processed, and the patch still cleanly applies to the latest revision of branch `default`.
msg214572 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-23 12:49
The patch needs .. versionadded:: 3.5 tags for the two new methods, and adding it to the skeleton whatsnew would be a good idea.  The committer can do these, but if you feel like updating the patch that would be great.
msg214591 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-23 15:31
I made the changes mentioned by r.david.murray

I am not sure if the modifications in ``Doc/whatsnew/3.5.rst`` are correct. I tried to follow the notes at the top of the file, but it's not clear to me if it should have gone into ``News/Misc`` or into ``Doc/whatsnew/3.5.rst``.

On another side-note: I attached this as an ``-r3`` file, but I could have replaced the existing patch as well. Which method is preferred? Replacing existing patches on the issue or adding new revisions?
msg246401 - (view) Author: James (JamesGuthrie) Date: 2015-07-07 11:24
What is the status of these changes? Apparently they were slated for inclusion in 3.5 but it looks as though they haven't hit yet - is there a reason for this, or was it just forgotten?
msg266869 - (view) Author: James Schneider (James Schneider) Date: 2016-06-02 03:21
I'd like to ask for a status on getting this merged? 

As a network administrator, these changes would have a magical effect on my code dealing with routing tables and ACL's.
msg269225 - (view) Author: Michel Albert (exhuma) * Date: 2016-06-25 10:30
I just realised that the latest patch on this no longer applies properly. I have fixed the issue and I am currently in the process of running the unit-tests which takes a while. Once those pass, I'll update some metadata and resubmit.
msg269226 - (view) Author: Michel Albert (exhuma) * Date: 2016-06-25 10:47
Test pass properly.

Is there anything else left to do?

Here's the fixed patch (net-in-net-r4.patch)
msg269228 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-06-25 10:58
Have you seen my comments on rietveld re you previous patch?
msg269229 - (view) Author: Michel Albert (exhuma) * Date: 2016-06-25 11:33
Updated patch, taking into account notes from the previous patch-reviews
msg269232 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-25 11:46
Thanks for the updated patch. Some comments from a quick review:

* We need tests for the TypeError branches in both methods

* +                            'of type %s' % type(other)

  type(other) -> type(other).__name__

* You can drop the XXX part in

  +XXX Instances of

* Perhaps code duplication mentioned by SilentGhost can be eliminated by using the operator module
msg269238 - (view) Author: Michel Albert (exhuma) * Date: 2016-06-25 13:46
I don't quite see how the operator module could help. I don't have much experience with it though, so I might be missing something...

I don't see how I can relate one check to the other. The example I gave in the patch review was the following:

With the existing implementation:

'192.168.1.0/25' subnet of '192.168.1.128/25' -> False
'192.168.1.0/25' supernet of '192.168.1.128/25' -> False

With the proposal to simply return "not subnet_of(...)" it would become:

'192.168.1.0/25' subnet of '192.168.1.128/25' -> False
'192.168.1.0/25' supernet of '192.168.1.128/25' -> True

which would be wrong.


I have now added the new test-cases for the TypeError and removed the code-duplication by introducing a new "private" function. Let me know what you think.


I am running all test cases again and I'll uploaded it once they finished.
msg269239 - (view) Author: Michel Albert (exhuma) * Date: 2016-06-25 14:35
New patch with proposed changes.
msg276208 - (view) Author: Michel Albert (exhuma) * Date: 2016-09-13 08:05
Are there any updates on this? Not sure if it's too late again to get it applied for the next Python (3.6) release?
msg279066 - (view) Author: James Schneider (James Schneider) Date: 2016-10-20 18:52
Please consider for implementation in 3.6. I'd love it even more for 3.5 but I don't think that will happen. With the latest patch, I don't believe there are any backwards-incompatible changes, though.
msg294244 - (view) Author: Aleksandr Balezin (gescheit) * Date: 2017-05-23 11:45
I've reviewed this patch and want to make some advices.
- hasattr is unwanted here. There is no any similar usage hasattr in this module. Also before hasattr there is a call of _ipversion method. If other is not instance of BaseNetwork it will raise AttributeError exception before hasattr check.
- It is not a good manner comparing thru "other.network_address >= self.network_address and other.broadcast_address <= self.broadcast_address"(see issue25430). Networks must be compared thru "other._prefixlen >= self._prefixlen and other.network._ip & self.netmask._ip == self.network._ip" for performance reason.
- _containment_check function is excessive. There is no common logic in supernet_of/subnet_of function except _ipversion and type checking. I think this two functions should be simple as:
def subnet_of(self, other):
    if self._version != other._version:
        raise TypeError('%s and %s are not of the same version' % (self, other))
    if other._prefixlen >= self._prefixlen and other.network._ip & self.netmask._ip == self.network._ip:
        return True
    return False
msg302798 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python triager) Date: 2017-09-23 19:06
Hello Michel,

Would you be able to convert your patch to a Github pull request?  It seemed like there was interest in merging this at some point, so maybe a PR would get it moving towards that again.
msg304683 - (view) Author: Mark Ignacio (Mark.Ignacio) Date: 2017-10-20 21:12
Hey Michel,

Are you still interested in pushing this patch through? It's be awesome if this got committed.
msg304768 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-22 21:39
New changeset 91dc64ba3f51100540b2ab6c6cd72c3bb18a6d49 by Antoine Pitrou (Cheryl Sabella) in branch 'master':
bpo-20825: Containment test for ip_network in ip_network.
https://github.com/python/cpython/commit/91dc64ba3f51100540b2ab6c6cd72c3bb18a6d49
msg304769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-10-22 21:40
This is now in 3.7.  Thanks to everyone who contributed!
History
Date User Action Args
2017-10-22 21:40:44pitrousetstatus: open -> closed
versions: + Python 3.7, - Python 3.8
messages: + msg304769

resolution: fixed
stage: patch review -> resolved
2017-10-22 21:39:51pitrousetmessages: + msg304768
2017-10-20 23:53:50cheryl.sabellasetpull_requests: + pull_request4035
2017-10-20 21:12:35Mark.Ignaciosetmessages: + msg304683
2017-10-20 20:59:16Mark.Ignaciosetnosy: + Mark.Ignacio

versions: + Python 3.8, - Python 3.6
2017-09-23 19:06:14cheryl.sabellasetnosy: + cheryl.sabella
messages: + msg302798
2017-05-23 11:45:08gescheitsetnosy: + gescheit
messages: + msg294244
2016-10-20 18:52:26James Schneidersetmessages: + msg279066
2016-09-13 08:05:32exhumasetmessages: + msg276208
2016-07-26 19:00:29berker.peksaglinkissue25431 superseder
2016-06-25 14:35:37exhumasetfiles: + net-in-net-r6.patch

messages: + msg269239
2016-06-25 13:46:49exhumasetmessages: + msg269238
2016-06-25 11:46:51berker.peksagsetmessages: + msg269232
2016-06-25 11:33:41exhumasetfiles: + net-in-net-r5.patch

messages: + msg269229
2016-06-25 10:58:52SilentGhostsetmessages: + msg269228
2016-06-25 10:47:15exhumasetfiles: + net-in-net-r4.patch

messages: + msg269226
2016-06-25 10:30:38exhumasetmessages: + msg269225
2016-06-10 13:08:59SilentGhostsetnosy: + SilentGhost
2016-06-10 12:37:34berker.peksagsetnosy: + berker.peksag
stage: patch review
type: enhancement

versions: + Python 3.6, - Python 3.5
2016-06-02 03:21:17James Schneidersetnosy: + James Schneider
messages: + msg266869
2015-07-07 11:24:38JamesGuthriesetnosy: + JamesGuthrie
messages: + msg246401
2014-03-23 15:31:15exhumasetfiles: + net-in-net-r3.patch

messages: + msg214591
2014-03-23 12:49:14r.david.murraysetmessages: + msg214572
2014-03-23 10:54:38exhumasetmessages: + msg214561
2014-03-11 20:22:52pmoodysetmessages: + msg213173
2014-03-11 19:38:29exhumasetmessages: + msg213169
2014-03-11 19:31:53pmoodysetassignee: pmoody
messages: + msg213167
2014-03-06 11:50:19exhumasetfiles: + net-in-net-r2.patch

messages: + msg212805
2014-03-06 03:10:10eric.smithsetnosy: + eric.smith
messages: + msg212792
2014-03-04 08:19:01exhumasetmessages: + msg212695
2014-03-02 15:23:38pitrousetmessages: + msg212560
2014-03-02 15:15:14r.david.murraysetnosy: + r.david.murray
messages: + msg212558
2014-03-02 14:07:12exhumasetmessages: + msg212552
2014-03-02 13:56:16pitrousetnosy: + pitrou
messages: + msg212551
2014-03-02 13:18:19exhumacreate