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

containment test for "ip_network in ip_network" #65024

Closed
exhuma mannequin opened this issue Mar 2, 2014 · 30 comments
Closed

containment test for "ip_network in ip_network" #65024

exhuma mannequin opened this issue Mar 2, 2014 · 30 comments
Labels
3.7 stdlib type-feature

Comments

@exhuma
Copy link
Mannequin

@exhuma exhuma mannequin commented Mar 2, 2014

BPO 20825
Nosy @ncoghlan, @pitrou, @ericvsmith, @bitdancer, @berkerpeksag, @gescheit, @exhuma, @csabella, @mark-ignacio
PRs
  • #4065
  • Files
  • net-in-net.patch
  • net-in-net-r2.patch
  • net-in-net-r3.patch
  • net-in-net-r4.patch
  • net-in-net-r5.patch
  • net-in-net-r6.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-10-22.21:40:44.019>
    created_at = <Date 2014-03-02.13:18:19.309>
    labels = ['3.7', 'type-feature', 'library']
    title = 'containment test for "ip_network in ip_network"'
    updated_at = <Date 2017-10-22.21:40:44.006>
    user = 'https://github.com/exhuma'

    bugs.python.org fields:

    activity = <Date 2017-10-22.21:40:44.006>
    actor = 'pitrou'
    assignee = 'pmoody'
    closed = True
    closed_date = <Date 2017-10-22.21:40:44.019>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2014-03-02.13:18:19.309>
    creator = 'exhuma'
    dependencies = []
    files = ['34266', '34292', '34588', '43534', '43535', '43537']
    hgrepos = []
    issue_num = 20825
    keywords = ['patch']
    message_count = 30.0
    messages = ['212550', '212551', '212552', '212558', '212560', '212695', '212792', '212805', '213167', '213169', '213173', '214561', '214572', '214591', '246401', '266869', '269225', '269226', '269228', '269229', '269232', '269238', '269239', '276208', '279066', '294244', '302798', '304683', '304768', '304769']
    nosy_count = 13.0
    nosy_names = ['ncoghlan', 'pitrou', 'eric.smith', 'pmoody', 'r.david.murray', 'SilentGhost', 'berker.peksag', 'gescheit', 'exhuma', 'JamesGuthrie', 'James Schneider', 'cheryl.sabella', 'Mark.Ignacio']
    pr_nums = ['4065']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20825'
    versions = ['Python 3.7']

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 2, 2014

    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.

    @exhuma exhuma mannequin added the stdlib label Mar 2, 2014
    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Mar 2, 2014

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

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 2, 2014

    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.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Mar 2, 2014

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

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Mar 2, 2014

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

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 4, 2014

    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.

    @ericvsmith
    Copy link
    Member

    @ericvsmith ericvsmith commented Mar 6, 2014

    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.

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 6, 2014

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

    It also contains the relevant docs and unit-tests.

    @pmoody
    Copy link
    Mannequin

    @pmoody pmoody mannequin commented Mar 11, 2014

    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/

    @pmoody pmoody mannequin self-assigned this Mar 11, 2014
    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 11, 2014

    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.

    @pmoody
    Copy link
    Mannequin

    @pmoody pmoody mannequin commented Mar 11, 2014

    Thanks!

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 23, 2014

    Hi again,

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

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Mar 23, 2014

    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.

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Mar 23, 2014

    I made the changes mentioned by r.david.murray

    I am not sure if the modifications in [Doc/whatsnew/3.5.rst](https://github.com/python/cpython/blob/main/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](https://github.com/python/cpython/blob/main/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?

    @JamesGuthrie
    Copy link
    Mannequin

    @JamesGuthrie JamesGuthrie mannequin commented Jul 7, 2015

    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?

    @JamesSchneider
    Copy link
    Mannequin

    @JamesSchneider JamesSchneider mannequin commented Jun 2, 2016

    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.

    @berkerpeksag berkerpeksag added the type-feature label Jun 10, 2016
    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Jun 25, 2016

    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.

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Jun 25, 2016

    Test pass properly.

    Is there anything else left to do?

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

    @SilentGhost
    Copy link
    Mannequin

    @SilentGhost SilentGhost mannequin commented Jun 25, 2016

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

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Jun 25, 2016

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

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Jun 25, 2016

    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

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Jun 25, 2016

    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.

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Jun 25, 2016

    New patch with proposed changes.

    @exhuma
    Copy link
    Mannequin Author

    @exhuma exhuma mannequin commented Sep 13, 2016

    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?

    @JamesSchneider
    Copy link
    Mannequin

    @JamesSchneider JamesSchneider mannequin commented Oct 20, 2016

    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.

    @gescheit
    Copy link
    Mannequin

    @gescheit gescheit mannequin commented May 23, 2017

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

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Sep 23, 2017

    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.

    @mark-ignacio mark-ignacio mannequin added the 3.8 label Oct 20, 2017
    @mark-ignacio
    Copy link
    Mannequin

    @mark-ignacio mark-ignacio mannequin commented Oct 20, 2017

    Hey Michel,

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

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Oct 22, 2017

    New changeset 91dc64b by Antoine Pitrou (Cheryl Sabella) in branch 'master':
    bpo-20825: Containment test for ip_network in ip_network.
    91dc64b

    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Oct 22, 2017

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

    @pitrou pitrou added 3.7 and removed 3.8 labels Oct 22, 2017
    @pitrou pitrou closed this as completed Oct 22, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants