adding an option for the filter ipsubnet and testing #40670

pierremahot · 2018-05-24T15:26:29Z

SUMMARY

Adding the option to request the index of a subnet in a bigger subnet

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

filter ipsubnet

ANSIBLE VERSION

ansible 2.5.2
  config file = ansible.cfg
  python version = 2.7.9 default, jun 29 2016, 13:08:31) [GCC 4.9.2]

ADDITIONAL INFORMATION

Need this feature to deduce easily a vlan for my switch

Examples:
---
- hosts: local
  tasks:
    - debug: msg="{{ '192.168.50.0/24' | ipsubnet('192.168.0.0/16') }}"
#display 51
    - debug: msg="{{ '10.10.10.8/30' | ipsubnet('192.168.0.0/16')}}
#display False
   - debug: msg="{{ '10.10.10.8/16' | ipsubnet('10.10.0.99/24')}}
#display False
   - debug: msg="{{ '10.10.0.9/30' | ipsubnet('10.10.0.99/24')}}
#display 3

webknjaz · 2018-05-24T16:27:23Z

lib/ansible/plugins/filter/ipaddr.py

 def ipsubnet(value, query='', index='x'):
+    strquery = str(query)


Please don't add anything before a docstring.

webknjaz · 2018-05-24T16:27:37Z

lib/ansible/plugins/filter/ipaddr.py

@@ -734,8 +738,7 @@ def ipsubnet(value, query='', index='x'):

    if not query:


better add it above this line

mattclay · 2018-05-25T03:43:37Z

CI failure in unit tests: https://app.shippable.com/github/ansible/ansible/runs/67119/3/tests

>       self.assertEqual(ipsubnet(address, subnet), False)
E       AssertionError: '35' != False

test/units/plugins/filter/test_ipaddr.py:490: AssertionError

webknjaz · 2018-05-26T21:24:45Z

lib/ansible/plugins/filter/ipaddr.py

@@ -735,10 +734,11 @@ def ipsubnet(value, query='', index='x'):
        value = netaddr.IPNetwork(v)
    except:
        return False
-
+    querystr = str(query)


Also, since in Python we use snake_case for variables/functions, please change this to query_string

webknjaz · 2018-05-26T21:27:33Z

lib/ansible/plugins/filter/ipaddr.py

+                v = ipaddr(query, 'subnet')
+
+            query = netaddr.IPNetwork(v)
+        except:


It is a bad practice to catch all exceptions wholesale. You mask different errors, which you should've processed correctly. Also this makes it almost impossible to debug misbehavior.

I just make the same code as before line 727 to 736 but I am not sure how to cut this the good way :

try: vtype = ipaddr(query, 'type') except: return False if vtype == 'address': try: v = ipaddr(query, 'cidr') except: return False elif vtype == 'network': try: v = ipaddr(query, 'subnet') except: return False query = netaddr.IPNetwork(v)

Yes, I see that, but it's not a good approach.
From https://github.com/drkjam/netaddr/blob/ebf714601e8cd6234575422f876d4d579b202b1a/netaddr/ip/__init__.py#L938 it looks like netaddr.IPNetwork(v) may raise AddrFormatError.

But if you want to catch all netaddr library's thrown exceptions, you may do:

from netaddr.core import AddrConversionError, AddrFormatError, NotRegisteredError NETADDR_ERRORS = AddrConversionError, AddrFormatError, NotRegisteredError ... try: ...your exceptional code here except NETADDR_ERRORS as err: process_or_log(err) # optionally raise/pass/return False (whatever fits)

I still insist on correct exception handling.

webknjaz · 2018-05-26T21:29:52Z

lib/ansible/plugins/filter/ipaddr.py

+        if network_in_network(query, value):
+            subnetlist = list(query.subnet(value.prefixlen))
+            for i in range(0, len(subnetlist)):
+                if (subnetlist[i] == value):


In Python, we don't wrap clauses with parentheses. Please remove them.

webknjaz · 2018-05-26T21:47:50Z

lib/ansible/plugins/filter/ipaddr.py

+
+        if network_in_network(query, value):
+            subnetlist = list(query.subnet(value.prefixlen))
+            for i in range(0, len(subnetlist)):


What you're trying to do is possible to achieve using "sliding window" technique with enumerate(), it will be possible to even drop dancing around "index+1" problem:

for i, subnet in enumerate(query.subnet(value.prefixlen)): if subnet == value: return str(i+1)

webknjaz · 2018-05-26T21:48:37Z

lib/ansible/plugins/filter/ipaddr.py

+            return False
+
+        if network_in_network(query, value):
+            subnetlist = list(query.subnet(value.prefixlen))


According to the doc, it's a generator: https://netaddr.readthedocs.io/en/latest/api.html#netaddr.IPNetwork.subnet

So you can iterate over it, no need to list() it. Please remove it.

webknjaz · 2018-05-26T21:51:14Z

lib/ansible/plugins/filter/ipaddr.py

+        except:
+            return False
+
+        if network_in_network(query, value):


I believe, you don't even need the if-wrapper, if query.subnet won't yield anything, the loop will be "skipped", just like your intention is.

ansibot · 2018-05-29T15:09:05Z

@pierremahot This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

pierremahot · 2018-05-30T06:17:54Z

It's my choice i prefert to have the first subnet to have the value 1 and not 0.
And with the enumerate i can return i insted of i + 1

webknjaz · 2018-05-31T12:37:14Z

lib/ansible/plugins/filter/ipaddr.py

@@ -782,7 +782,7 @@ def ipsubnet(value, query='', index='x'):
        except:
            return False

-        for i, subnet in enumerate(query.subnet(value.prefixlen)):
+        for i, subnet in enumerate(query.subnet(value.prefixlen), 1):


webknjaz · 2018-05-31T12:39:14Z

I've restarted failed CI jobs.

webknjaz · 2018-05-31T13:50:55Z

test/units/plugins/filter/test_ipaddr.py

@@ -473,3 +473,32 @@ def test_cidr_merge(self):
        subnets = ['1.12.1.1', '1.12.1.255']
        self.assertEqual(cidr_merge(subnets), ['1.12.1.1/32', '1.12.1.255/32'])
        self.assertEqual(cidr_merge(subnets, 'span'), '1.12.1.0/24')
+
+    def test_ipsubnet(self):


where's negative tests?

webknjaz · 2018-05-31T13:52:43Z

test/units/plugins/filter/test_ipaddr.py

+        self.assertEqual(ipsubnet(address, subnet), False)
+        address = '192.168.144.5'
+        subnet = '192.168.0.0/16'
+        self.assertEqual(ipsubnet(address), '192.168.144.5/32')


Please keep your code style consistent: first you assign vars for some time, then you pass lots of function args directly. There's a better way.

webknjaz · 2018-08-14T11:05:59Z

lib/ansible/plugins/filter/ipaddr.py

+                v = ipaddr(query, 'subnet')
+            query = netaddr.IPNetwork(v)
+        except NETADDR_ERRORS as err:
+            raise errors.AnsibleFilterError(err)


This is minor, but I like to keep exception chain when re-raising new ones. This is how you do it in forward-compatible manner:

from ansible.module_utils import six ... except NETADDR_ERRORS as err: six.raise_from(errors.AnsibleFilterError(err), err)

webknjaz

👍 🎉

pierremahot · 2018-08-14T11:40:34Z

I have a problème with the exception :( doesn't look to work it return 1

- hosts: localhost
  tasks:
    - debug:
        msg: "{{ '192.168.1.1/24' | ipsubnet('-5') }}"

result

ansible-playbook test.yml
 [WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'


PLAY [localhost] *****************************************************************************************************************************************************************************

TASK [Gathering Facts] ***********************************************************************************************************************************************************************
ok: [localhost]

TASK [debug] *********************************************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "1"
}

PLAY RECAP ***********************************************************************************************************************************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=0

webknjaz · 2018-08-14T11:45:45Z

@pierremahot can you add that to integration tests?

webknjaz · 2018-08-14T11:46:46Z

Also, what do you mean by exception?

webknjaz · 2018-08-14T11:48:48Z

test/units/plugins/filter/test_ipaddr.py

+            self._test_ipsubnet(args, res)
+
+    def _test_ipsubnet(self, ipsubnet_args, expected_result):
+        self.assertEqual(ipsubnet(*ipsubnet_args), expected_result)


I'd also add some test cases with assertRaises

I think

vtype = ipaddr(query, 'type') if vtype == 'address': v = ipaddr(query, 'cidr') elif vtype == 'network': v = ipaddr(query, 'subnet') else: return False

Can't raise error on :

query = netaddr.IPNetwork(v) for i, subnet in enumerate(query.subnet(value.prefixlen), 1)

i don't find the case

You could mock.patch it to do so :)

Also, look out for netaddr docs for the cases when it'll raise the exception.

pierremahot · 2018-08-14T12:14:52Z

I can't have assertion error because of the way ipaddr works

webknjaz · 2018-08-14T12:25:13Z

test/units/plugins/filter/test_ipaddr.py

+        expected = 'An Exception indicating a failure to convert between address types or notations.'
+        with self.assertRaises(AddrConversionError) as exc:
+            ipsubnet('192.168.144.5', '192.168.8.1.5')
+            self.assertEqual(exc.exception.message, expected)


This needs to go outside of CM. If ipsubnet will raise exception it will interrupt code block within with.

webknjaz · 2018-08-14T12:27:08Z

test/units/plugins/filter/test_ipaddr.py

+        self.assertEqual(ipsubnet(*ipsubnet_args), expected_result)
+
+        expected = 'An Exception indicating a network address is not correctly formatted.'
+        with self.assertRaises(AddrConversionError) as exc:


I think, you're looking for self.assertRaisesRegexp(AddrConversionError, expected)

I don't find case to raise an error

pierremahot · 2018-08-14T12:40:48Z

I think ipaddr is the function that can raise error but it filter already the input of netaddr i don't find case where it raise an error

webknjaz · 2018-08-14T13:09:43Z

Yeah, you're right: ipaddr brutally swallows all exceptions.

webknjaz · 2018-08-14T15:04:35Z

test/units/plugins/filter/test_ipaddr.py

@@ -541,3 +537,12 @@ def test_ipsubnet(self):

    def _test_ipsubnet(self, ipsubnet_args, expected_result):
        self.assertEqual(ipsubnet(*ipsubnet_args), expected_result)
+        expected = 'You must pass a valid subnet or IP address; invalid_subnet is invalid'
+        with self.assertRaises(AnsibleFilterError) as exc:


Please use assertRaisesRegexp instead.

ansibot added affects_2.6 This issue/PR affects Ansible v2.6 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 24, 2018

pierremahot mentioned this pull request May 24, 2018

adding an option for the filter ipsubnet #40141

Closed

ansibot added the test This PR relates to tests. label May 24, 2018

samdoran added networking Network category and removed needs_triage Needs a first human triage before being processed. labels May 24, 2018

webknjaz reviewed May 24, 2018

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 24, 2018

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label May 25, 2018

pierremahot closed this May 25, 2018

pierremahot reopened this May 25, 2018

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label May 25, 2018

pierremahot closed this May 26, 2018

pierremahot reopened this May 26, 2018

webknjaz reviewed May 26, 2018

View reviewed changes

webknjaz reviewed May 31, 2018

View reviewed changes

webknjaz approved these changes Aug 14, 2018

View reviewed changes

webknjaz reviewed Aug 14, 2018

View reviewed changes

add re-raising exception chain as webknjaz tell

498191c

webknjaz approved these changes Aug 14, 2018

View reviewed changes

adding exception test

b6a199a

webknjaz reviewed Aug 14, 2018

View reviewed changes

testing assertion

30cf9b1

webknjaz reviewed Aug 14, 2018

View reviewed changes

pierremahot added 2 commits August 14, 2018 14:35

no raise possible

1ed1326

update test and code

6e6bf9a

raise error when out of subnet or wrong subnet input

87fb43a

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 14, 2018

update doc for raising error

7e8090e

ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 14, 2018

webknjaz reviewed Aug 14, 2018

View reviewed changes

use asserRaisesRegexp

a8f4872

webknjaz approved these changes Aug 14, 2018

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 22, 2018

gundalow merged commit d11e078 into ansible:devel Aug 30, 2018

ansible locked and limited conversation to collaborators Jul 22, 2019

		def ipsubnet(value, query='', index='x'):
		strquery = str(query)

		@@ -734,8 +738,7 @@ def ipsubnet(value, query='', index='x'):

		if not query:

adding an option for the filter ipsubnet and testing #40670

adding an option for the filter ipsubnet and testing #40670

Conversation

pierremahot commented May 24, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierremahot May 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz May 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented May 29, 2018

pierremahot commented May 30, 2018 • edited

Choose a reason for hiding this comment

webknjaz commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

pierremahot commented Aug 14, 2018

webknjaz commented Aug 14, 2018

webknjaz commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierremahot commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierremahot commented Aug 14, 2018

webknjaz commented Aug 14, 2018

Choose a reason for hiding this comment

pierremahot May 29, 2018 •

edited

webknjaz May 26, 2018 •

edited

pierremahot commented May 30, 2018 •

edited