[issue20825] containment test for "ip_network in ip_network"

[Antoine Pitrou]

Antoine Pitrou  added the comment:

This is now in 3.7.  Thanks to everyone who contributed!

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.7 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Antoine Pitrou]

Antoine Pitrou  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Cheryl Sabella]

Change by Cheryl Sabella :


--
pull_requests: +4035

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Mark Ignacio]

Mark Ignacio  added the comment:

Hey Michel,

Are you still interested in pushing this patch through? It's be awesome if this 
got committed.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Mark Ignacio]

Change by Mark Ignacio :


--
nosy: +Mark.Ignacio
versions: +Python 3.8 -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Cheryl Sabella]

Cheryl Sabella added the comment:

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.

--
nosy: +csabella

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Aleksandr Balezin]

Aleksandr Balezin added the comment:

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

--
nosy: +gescheit

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[James Schneider]

James Schneider added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Michel Albert]

Michel Albert added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Michel Albert]

Michel Albert added the comment:

New patch with proposed changes.

--
Added file: http://bugs.python.org/file43537/net-in-net-r6.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Berker Peksag]

Berker Peksag added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Michel Albert]

Michel Albert added the comment:

Updated patch, taking into account notes from the previous patch-reviews

--
Added file: http://bugs.python.org/file43535/net-in-net-r5.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[SilentGhost]

SilentGhost added the comment:

Have you seen my comments on rietveld re you previous patch?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Michel Albert]

Michel Albert added the comment:

Test pass properly.

Is there anything else left to do?

Here's the fixed patch (net-in-net-r4.patch)

--
Added file: http://bugs.python.org/file43534/net-in-net-r4.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Michel Albert]

Michel Albert added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[SilentGhost]

Changes by SilentGhost :


--
nosy: +SilentGhost

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[Berker Peksag]

Changes by Berker Peksag :


--
nosy: +berker.peksag
stage:  -> patch review
type:  -> enhancement
versions: +Python 3.6 -Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for "ip_network in ip_network"

[James Schneider]

James Schneider added the comment:

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.

--
nosy: +James Schneider

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[James]

James added the comment:

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?

--
nosy: +JamesGuthrie

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

Hi again,

The contribution agreement has been processed, and the patch still cleanly 
applies to the latest revision of branch `default`.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

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?

--
Added file: http://bugs.python.org/file34588/net-in-net-r3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[pmoody]

pmoody added the comment:

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/

--
assignee:  - pmoody

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[pmoody]

pmoody added the comment:

Thanks!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

Here's a new patch implementing both ``subnet_of`` and ``supernet_of``.

It also contains the relevant docs and unit-tests.

--
Added file: http://bugs.python.org/file34292/net-in-net-r2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--
nosy: +eric.smith

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

New submission from Michel Albert:

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``.

--
components: Library (Lib)
files: net-in-net.patch
keywords: patch
messages: 212550
nosy: exhuma, ncoghlan, pmoody
priority: normal
severity: normal
status: open
title: containment test for ip_network in ip_network
versions: Python 3.5
Added file: http://bugs.python.org/file34266/net-in-net.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Antoine Pitrou]

Antoine Pitrou added the comment:

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).

--
nosy: +pitrou

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Michel Albert]

Michel Albert added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[R. David Murray]

R. David Murray added the comment:

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).

--
nosy: +r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20825] containment test for ip_network in ip_network

[Antoine Pitrou]

Antoine Pitrou added the comment:

 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))

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20825
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com