From 57c5cd4336997dfd3053280fa5935f36742d8de9 Mon Sep 17 00:00:00 2001 From: Tim Way <1091435+timway@users.noreply.github.com> Date: Wed, 31 Aug 2022 11:09:35 -0500 Subject: [PATCH] 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 --- .../fragments/ipsubnet_performance.yaml | 14 +++ docs/ansible.utils.ipsubnet_filter.rst | 2 +- plugins/filter/ipsubnet.py | 116 ++++++++++-------- tests/integration/requirements.txt | 1 + .../utils_ipaddr_filter/tasks/ipsubnet.yaml | 16 +++ tests/unit/plugins/filter/test_ipaddr.py | 14 +-- tests/unit/plugins/filter/test_ipsubnet.py | 48 ++++++++ 7 files changed, 153 insertions(+), 58 deletions(-) create mode 100644 changelogs/fragments/ipsubnet_performance.yaml diff --git a/changelogs/fragments/ipsubnet_performance.yaml b/changelogs/fragments/ipsubnet_performance.yaml new file mode 100644 index 0000000..f60ebbc --- /dev/null +++ b/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. diff --git a/docs/ansible.utils.ipsubnet_filter.rst b/docs/ansible.utils.ipsubnet_filter.rst index 12422ca..c73244f 100644 --- a/docs/ansible.utils.ipsubnet_filter.rst +++ b/docs/ansible.utils.ipsubnet_filter.rst @@ -40,7 +40,7 @@ Parameters index
- string + integer
diff --git a/plugins/filter/ipsubnet.py b/plugins/filter/ipsubnet.py index f127a36..85bedb4 100644 --- a/plugins/filter/ipsubnet.py +++ b/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") - if vtype == "address": - v = ipaddr(value, "cidr") - elif vtype == "network": - v = ipaddr(value, "subnet") - - value = netaddr.IPNetwork(v) - except Exception: + vtype = ipaddr(value, "type") + if not vtype: 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(): - vsize = ipaddr(v, "size") + vtotalbits = 128 if value.version == 6 else 32 + if query.isdigit(): query = int(query) + if query < 0 or query > vtotalbits: + return False - try: - float(index) - index = int(index) + if index is None: + if vtype == "address": + 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: - try: - return str(list(value.subnet(query))[index]) - except Exception: - return False - - elif vsize == 1: - try: - return str(value.supernet(query)[index]) - except Exception: - return False - - except Exception: - if vsize > 1: - try: - return str(len(list(value.subnet(query)))) - except Exception: - return False - - elif vsize == 1: - try: - return str(value.supernet(query)[0]) - except Exception: - return False - - elif query_string: + index = int(index) + if vtype == "address": + if index > vtotalbits + 1 - index or index < query - vtotalbits: + return False + return to_text(value.supernet(query)[index]) + elif vtype == "network": + if index < 0: + index = index + 2 ** (query - value.prefixlen) + return to_text( + netaddr.IPNetwork( + to_text( + netaddr.IPAddress( + value.network.value + (index << vtotalbits - query), + ), + ) + + "/" + + to_text(query), + ), + ) + else: 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 subnet == value: - return str(i) + if ( + value.value >> vtotalbits - query.prefixlen + == 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 diff --git a/tests/integration/requirements.txt b/tests/integration/requirements.txt index 279789b..118f11d 100644 --- a/tests/integration/requirements.txt +++ b/tests/integration/requirements.txt @@ -1,3 +1,4 @@ jsonschema ; python_version >= '2.7' +netaddr ttp textfsm diff --git a/tests/integration/targets/utils_ipaddr_filter/tasks/ipsubnet.yaml b/tests/integration/targets/utils_ipaddr_filter/tasks/ipsubnet.yaml index a134c38..76740d3 100644 --- a/tests/integration/targets/utils_ipaddr_filter/tasks/ipsubnet.yaml +++ b/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' diff --git a/tests/unit/plugins/filter/test_ipaddr.py b/tests/unit/plugins/filter/test_ipaddr.py index a310244..72783c1 100644 --- a/tests/unit/plugins/filter/test_ipaddr.py +++ b/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", diff --git a/tests/unit/plugins/filter/test_ipsubnet.py b/tests/unit/plugins/filter/test_ipsubnet.py index 6774d35..b790f60 100644 --- a/tests/unit/plugins/filter/test_ipsubnet.py +++ b/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]