Switch To Calculating Networking Information Directly For Performance (#146)

* Add Unit Tests To Capture Failures From 'subnet' Generator

The netaddr library returns a generator for the 'subnet' call. This works great until you use larger networks. While it is uncommon to encounter it in IPv4 usage it is trivial to hit it in IPv6.

* Switch To Calculating Networking Information Directly For Performance

This replaces the inefficient generator for 'subnet' and uses math to determine the result directly. Since a list is not returned directly to the client in the implemented cases this works great and is fast.

A further optimization at least on the logic of this might be to break the different cases implemented by the filter out into unique functions. I did not do this yet because I wanted to get feedback on this direction.

* Changelog Fragment For PR / Bugfix

Adding changelog fragment that references source issue.

* Dropping Python 3.7 Bypass Removes Need For 'sys' Module

A test for ipsubnet was bypassed under 3.7 because of an inconsistent return value w/3.6 and 2.7. I removed the bypass and changed the behavior of the filter to raise an AnsibleFilterError in all versions of Python.

* Add A Pair of Integration Tests

These demonstrate the issue with the current implementation and would normally stall out while building the list of possible subnets from the generator.

* Address Changelog Feedback

I kept the performance item as a bugfix but bumped the typing to a minor change.

* Add 'netaddr' To Integration Test 'requirements.txt'

* The `ansible-test integration --docker` requires this line in requirements.txt to pass the 'netaddr' related tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Replace str -> to_text

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Kate Case <this.is@katherineca.se>

changelogs/fragments/ipsubnet_performance.yaml

@@ -0,0 +1,14 @@
+---
+bugfixes:
+  - >-
+    ipsubnet - interacting with large subnets could cause performance
+    constraints. the result would be the system would appear to hang while
+    it built out a list of all possible subnets or stepped through all
+    possible subnets one at a time. when sending a prefix that is a supernet
+    of the passed in network the behavior wasn't consistent. this now returns
+    an AnsibleFilterError in that scenario across all python releases.
+    (https://github.com/ansible-collections/ansible.utils/issues/132)
+minor_changes:
+  - >-
+    ipsubnet - the index parameter should only ever be an integer if it is
+    provided. this changes the argument type from str to int.

docs/ansible.utils.ipsubnet_filter.rst

@@ -40,7 +40,7 @@ Parameters
                     <b>index</b>
                     <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a>
                     <div style="font-size: small">
-                        <span style="color: purple">string</span>
+                        <span style="color: purple">integer</span>
                     </div>
                 </td>
                 <td>

plugins/filter/ipsubnet.py

@@ -24,6 +24,9 @@ from ansible_collections.ansible.utils.plugins.plugin_utils.base.ipaddr_utils im
 __metaclass__ = type
 
 
+from ansible.module_utils._text import to_text
+
+
 try:
     from jinja2.filters import pass_environment
 except ImportError:
@@ -69,7 +72,7 @@ DOCUMENTATION = """
             description: |
                 The second argument of the ipsubnet() filter is an index number; by specifying it you can get a new subnet
                 with the specified index.
-            type: str
+            type: int
     notes:
 """
 
@@ -254,71 +257,88 @@ def _ipsubnet(*args, **kwargs):
     return ipsubnet(**updated_data)
 
 
-def ipsubnet(value, query="", index="x"):
+def ipsubnet(value, query="", index=None):
     """Manipulate IPv4/IPv6 subnets"""
 
-    try:
+    vtype = ipaddr(value, "type")
-        vtype = ipaddr(value, "type")
+    if not vtype:
-        if vtype == "address":
-            v = ipaddr(value, "cidr")
-        elif vtype == "network":
-            v = ipaddr(value, "subnet")
-
-        value = netaddr.IPNetwork(v)
-    except Exception:
         return False
-    query_string = str(query)
+    elif vtype == "address":
+        v = ipaddr(value, "cidr")
+    elif vtype == "network":
+        v = ipaddr(value, "subnet")
+    value = netaddr.IPNetwork(v).cidr
+    vtype = ipaddr(value, "type")
+
     if not query:
-        return str(value)
+        return to_text(value)
 
-    elif query_string.isdigit():
+    vtotalbits = 128 if value.version == 6 else 32
-        vsize = ipaddr(v, "size")
+    if query.isdigit():
         query = int(query)
+        if query < 0 or query > vtotalbits:
+            return False
 
-        try:
+        if index is None:
-            float(index)
+            if vtype == "address":
-            index = int(index)
+                return to_text(value.supernet(query)[0])
+            elif vtype == "network":
+                if query - value.prefixlen < 0:
+                    msg = "Requested subnet size of {0} is invalid".format(
+                        to_text(query),
+                    )
+                    raise AnsibleFilterError(msg)
+                return to_text(2 ** (query - value.prefixlen))
 
-            if vsize > 1:
+        index = int(index)
-                try:
+        if vtype == "address":
-                    return str(list(value.subnet(query))[index])
+            if index > vtotalbits + 1 - index or index < query - vtotalbits:
-                except Exception:
+                return False
-                    return False
+            return to_text(value.supernet(query)[index])
-
+        elif vtype == "network":
-            elif vsize == 1:
+            if index < 0:
-                try:
+                index = index + 2 ** (query - value.prefixlen)
-                    return str(value.supernet(query)[index])
+            return to_text(
-                except Exception:
+                netaddr.IPNetwork(
-                    return False
+                    to_text(
-
+                        netaddr.IPAddress(
-        except Exception:
+                            value.network.value + (index << vtotalbits - query),
-            if vsize > 1:
+                        ),
-                try:
+                    )
-                    return str(len(list(value.subnet(query))))
+                    + "/"
-                except Exception:
+                    + to_text(query),
-                    return False
+                ),
-
+            )
-            elif vsize == 1:
+    else:
-                try:
-                    return str(value.supernet(query)[0])
-                except Exception:
-                    return False
-
-    elif query_string:
         vtype = ipaddr(query, "type")
         if vtype == "address":
             v = ipaddr(query, "cidr")
         elif vtype == "network":
             v = ipaddr(query, "subnet")
         else:
-            msg = "You must pass a valid subnet or IP address; {0} is invalid".format(query_string)
+            msg = "You must pass a valid subnet or IP address; {0} is invalid".format(
+                to_text(query),
+            )
             raise AnsibleFilterError(msg)
         query = netaddr.IPNetwork(v)
-        for i, subnet in enumerate(query.subnet(value.prefixlen), 1):
+        if (
-            if subnet == value:
+            value.value >> vtotalbits - query.prefixlen
-                return str(i)
+            == query.value >> vtotalbits - query.prefixlen
+        ):
+            return to_text(
+                (
+                    (
+                        value.value
+                        & 2 ** (value.prefixlen - query.prefixlen) - 1
+                        << (vtotalbits - value.prefixlen)
+                    )
+                    >> (vtotalbits - value.prefixlen)
+                )
+                + 1,
+            )
         msg = "{0} is not in the subnet {1}".format(value.cidr, query.cidr)
         raise AnsibleFilterError(msg)
+
     return False
 
 

tests/integration/requirements.txt

@@ -1,3 +1,4 @@
 jsonschema ; python_version >= '2.7'
+netaddr
 ttp
 textfsm

tests/integration/targets/utils_ipaddr_filter/tasks/ipsubnet.yaml

@@ -62,3 +62,19 @@
 - name: Assert result for ipsubnet.
   assert:
     that: "{{ result8 == '36870' }}"
+
+- name: Small subnets from a large supernet should return quickly
+  ansible.builtin.set_fact:
+    result9: "{{ '2001:db8::/56' | ansible.utils.ipsubnet(120, 0) }}"
+
+- name: Assert result for ipsubnet.
+  assert:
+    that: result9 == '2001:db8::/120'
+
+- name: Small subnets from a large supernet should return quickly at index
+  ansible.builtin.set_fact:
+    result10: "{{ '2001:db8::/56' | ansible.utils.ipsubnet(120, 4) }}"
+
+- name: Assert result for ipsubnet.
+  assert:
+    that: result10 == '2001:db8::400/120'

tests/unit/plugins/filter/test_ipaddr.py

@@ -11,7 +11,6 @@ from __future__ import absolute_import, division, print_function
 
 __metaclass__ = type
 
-import sys
 import unittest
 
 import pytest
@@ -498,7 +497,6 @@ class TestIpFilter(unittest.TestCase):
     def test_ipsubnet(self):
         test_cases = (
             (("1.1.1.1/24", "30"), "64"),
-            (("1.1.1.1/25", "24"), "0"),
             (("1.12.1.34/32", "1.12.1.34/24"), "35"),
             (("192.168.50.0/24", "192.168.0.0/16"), "51"),
             (("192.168.144.5", "192.168.0.0/16"), "36870"),
@@ -528,15 +526,13 @@ class TestIpFilter(unittest.TestCase):
             self._test_ipsubnet(args, res)
 
     def _test_ipsubnet(self, ipsubnet_args, expected_result):
-        if (
-            ipsubnet_args == ("1.1.1.1/25", "24")
-            and expected_result == "0"
-            and sys.version_info >= (3, 7)
-        ):
-            return  # fails in netaddr on Python 3.7+
-
         self.assertEqual(ipsubnet(*ipsubnet_args), expected_result)
 
+        expected = "Requested subnet size of 24 is invalid"
+        with self.assertRaises(AnsibleFilterError) as exc:
+            ipsubnet("1.1.1.1/25", "24")
+        self.assertEqual(exc.exception.message, expected)
+
         with self.assertRaisesRegexp(
             AnsibleFilterError,
             "You must pass a valid subnet or IP address; invalid_subnet is invalid",

tests/unit/plugins/filter/test_ipsubnet.py

@@ -73,6 +73,54 @@ class TestIpSubnet(unittest.TestCase):
         result = _ipsubnet(*args)
         self.assertEqual(result, "192.168.144.4/31")
 
+    def test_ipvsubnet_filter_lots_of_subnets_v4_bottom1(self):
+        """Use the subnets call to see if it slows way down with v4"""
+        args = ["", "0.0.0.0/1", "31", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "127.255.255.254/31")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v4_bottom2(self):
+        """Use the subnets call to see if it slows way down with v4"""
+        args = ["", "1.0.0.0/1", "3", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "96.0.0.0/3")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v4_top1(self):
+        """Use the subnets call to see if it slows way down with v4"""
+        args = ["", "128.0.0.0/1", "31", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "255.255.255.254/31")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v4_top2(self):
+        """Use the subnets call to see if it slows way down with v4"""
+        args = ["", "130.0.0.0/1", "31", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "255.255.255.254/31")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v6_bottom1(self):
+        """Use the subnets call to see if it slows way down with v6"""
+        args = ["", "8000::/1", "127", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "ffff:ffff:ffff:ffff:ffff:ffff:ffff:fffe/127")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v6_bottom2(self):
+        """Use the subnets call to see if it slows way down with v6"""
+        args = ["", "9000::/1", "127", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "ffff:ffff:ffff:ffff:ffff:ffff:ffff:fffe/127")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v6_top1(self):
+        """Use the subnets call to see if it slows way down with v6"""
+        args = ["", "::/1", "127", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "7fff:ffff:ffff:ffff:ffff:ffff:ffff:fffe/127")
+
+    def test_ipvsubnet_filter_lots_of_subnets_v6_top2(self):
+        """Use the subnets call to see if it slows way down with v6"""
+        args = ["", "1000::/1", "127", -1]
+        result = _ipsubnet(*args)
+        self.assertEqual(result, "7fff:ffff:ffff:ffff:ffff:ffff:ffff:fffe/127")
+
     def test_ipvsubnet_filter_rank_address_in_subnet(self):
         """The rank of the IP in the subnet (the IP is the 36870nth /32 of the subnet)"""
         args = ["", address, subnet]