[pfx_filter]: Add a prefix mask by default in pfx_filter, when there is no one (#4860)

If some table with a list of tuples (interface name, ip prefix) has ip prefixes without a mask length, it will cause issues in SONiC. For example quagga and frr will treat ipv4 address without a mask, so "10.20.30.40" address will be treated as "10.0.0.0/8", which is dangerous.

The fix here is that when pfx_filter get a tuple (interface name, ip prefix), where the ip prefix doesn't have prefix mask length, add a mask by default: "/32 for ipv4 addresses, /128 for ipv6 addresses".

Co-authored-by: Pavel Shirshov <pavel.contrib@gmail.com>

src/sonic-bgpcfgd/app/template.py

@@ -0,0 +1,108 @@
+from collections import OrderedDict
+from functools import partial
+
+import jinja2
+import netaddr
+
+from .log import log_err
+
+class TemplateFabric(object):
+    """ Fabric for rendering jinja2 templates """
+    def __init__(self, template_path = '/usr/share/sonic/templates'):
+        j2_template_paths = [template_path]
+        j2_loader = jinja2.FileSystemLoader(j2_template_paths)
+        j2_env = jinja2.Environment(loader=j2_loader, trim_blocks=False)
+        j2_env.filters['ipv4'] = self.is_ipv4
+        j2_env.filters['ipv6'] = self.is_ipv6
+        j2_env.filters['pfx_filter'] = self.pfx_filter
+        for attr in ['ip', 'network', 'prefixlen', 'netmask']:
+            j2_env.filters[attr] = partial(self.prefix_attr, attr)
+        self.env = j2_env
+
+    def from_file(self, filename):
+        """
+        Read a template from a file
+        :param filename: filename of the file. Type String
+        :return: Jinja2 template object
+        """
+        return self.env.get_template(filename)
+
+    def from_string(self, tmpl):
+        """
+        Read a template from a string
+        :param tmpl: Text representation of Jinja2 template
+        :return: Jinja2 template object
+        """
+        return self.env.from_string(tmpl)
+
+    @staticmethod
+    def is_ipv4(value):
+        """ Return True if the value is an ipv4 address """
+        if not value:
+            return False
+        if isinstance(value, netaddr.IPNetwork):
+            addr = value
+        else:
+            try:
+                addr = netaddr.IPNetwork(str(value))
+            except (netaddr.NotRegisteredError, netaddr.AddrFormatError, netaddr.AddrConversionError):
+                return False
+        return addr.version == 4
+
+    @staticmethod
+    def is_ipv6(value):
+        """ Return True if the value is an ipv6 address """
+        if not value:
+            return False
+        if isinstance(value, netaddr.IPNetwork):
+            addr = value
+        else:
+            try:
+                addr = netaddr.IPNetwork(str(value))
+            except (netaddr.NotRegisteredError, netaddr.AddrFormatError, netaddr.AddrConversionError):
+                return False
+        return addr.version == 6
+
+    @staticmethod
+    def prefix_attr(attr, value):
+        """
+        Extract attribute from IPNetwork object
+        :param attr: attribute to extract
+        :param value: the string representation of ip prefix which will be converted to IPNetwork.
+        :return: the value of the extracted attribute
+        """
+        if not value:
+            return None
+        else:
+            try:
+                prefix = netaddr.IPNetwork(str(value))
+            except (netaddr.NotRegisteredError, netaddr.AddrFormatError, netaddr.AddrConversionError):
+                return None
+        return str(getattr(prefix, attr))
+
+    @staticmethod
+    def pfx_filter(value):
+        """INTERFACE Table can have keys in one of the two formats:
+           string or tuple - This filter skips the string keys and only
+           take into account the tuple.
+           For eg - VLAN_INTERFACE|Vlan1000 vs VLAN_INTERFACE|Vlan1000|192.168.0.1/21
+        """
+        table = OrderedDict()
+
+        if not value:
+            return table
+
+        for key, val in value.items():
+            if not isinstance(key, tuple):
+                continue
+            intf, ip_address = key
+            if '/' not in ip_address:
+                if TemplateFabric.is_ipv4(ip_address):
+                    table[(intf, "%s/32" % ip_address)] = val
+                elif TemplateFabric.is_ipv6(ip_address):
+                    table[(intf, "%s/128" % ip_address)] = val
+                else:
+                    log_err("'%s' is invalid ip address" % ip_address)
+            else:
+                table[key] = val
+        return table

src/sonic-bgpcfgd/tests/test_pfx_filter.py

@@ -0,0 +1,139 @@
+from app.template import TemplateFabric
+from collections import OrderedDict
+import pytest
+
+
+def test_pfx_filter_none():
+    res = TemplateFabric.pfx_filter(None)
+    assert isinstance(res, OrderedDict) and len(res) == 0
+
+def test_pfx_filter_empty_tuple():
+    res = TemplateFabric.pfx_filter(())
+    assert isinstance(res, OrderedDict) and len(res) == 0
+
+def test_pfx_filter_empty_list():
+    res = TemplateFabric.pfx_filter([])
+    assert isinstance(res, OrderedDict) and len(res) == 0
+
+def test_pfx_filter_empty_dict():
+    res = TemplateFabric.pfx_filter({})
+    assert isinstance(res, OrderedDict) and len(res) == 0
+
+def test_pfx_filter_strings():
+    src = {
+        'Loopback0': {},
+        'Loopback1': {},
+    }
+    expected = OrderedDict([])
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+def test_pfx_filter_mixed_keys():
+    src = {
+        'Loopback0': {},
+        ('Loopback0', '11.11.11.11/32'): {},
+        'Loopback1': {},
+        ('Loopback1', '55.55.55.55/32'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback1', '55.55.55.55/32'), {}),
+            (('Loopback0', '11.11.11.11/32'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+
+def test_pfx_filter_pfx_v4_w_mask():
+    src = {
+        ('Loopback0', '11.11.11.11/32'): {},
+        ('Loopback1', '55.55.55.55/32'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback1', '55.55.55.55/32'), {}),
+            (('Loopback0', '11.11.11.11/32'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+def test_pfx_filter_pfx_v6_w_mask():
+    src = {
+        ('Loopback0', 'fc00::/128'): {},
+        ('Loopback1', 'fc00::1/128'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback0', 'fc00::/128'), {}),
+            (('Loopback1', 'fc00::1/128'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+def test_pfx_filter_pfx_v4_no_mask():
+    src = {
+        ('Loopback0', '11.11.11.11'): {},
+        ('Loopback1', '55.55.55.55'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback1', '55.55.55.55/32'), {}),
+            (('Loopback0', '11.11.11.11/32'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+def test_pfx_filter_pfx_v6_no_mask():
+    src = {
+        ('Loopback0', 'fc00::'): {},
+        ('Loopback1', 'fc00::1'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback0', 'fc00::/128'), {}),
+            (('Loopback1', 'fc00::1/128'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+
+def test_pfx_filter_pfx_comprehensive():
+    src = {
+        'Loopback0': {},
+        ('Loopback0', 'fc00::'): {},
+        'Loopback1': {},
+        ('Loopback1', 'fc00::1/128'): {},
+        ('Loopback2', '11.11.11.11/32'): {},
+        ('Loopback3', '55.55.55.55'): {},
+        'Loopback2': {},
+        'Loopback3': {},
+        ('Loopback5', '22.22.22.1/24'): {},
+        ('Loopback6', 'fc00::55/64'): {},
+    }
+    expected = OrderedDict(
+        [
+            (('Loopback1', 'fc00::1/128'), {}),
+            (('Loopback3', '55.55.55.55/32'), {}),
+            (('Loopback6', 'fc00::55/64'), {}),
+            (('Loopback2', '11.11.11.11/32'), {}),
+            (('Loopback0', 'fc00::/128'), {}),
+            (('Loopback5', '22.22.22.1/24'), {}),
+        ]
+    )
+    res = TemplateFabric.pfx_filter(src)
+    assert res == expected
+
+@pytest.fixture
+def test_pfx_filter_wrong_ip(caplog):
+    src = {
+        ('Loopback0', 'wrong_ip'): {},
+    }
+    res = TemplateFabric.pfx_filter(src)
+    assert "'wrong_ip' is invalid ip address" in caplog.text
+    assert isinstance(res, OrderedDict) and len(res) == 0
+

src/sonic-config-engine/sonic-cfggen

@@ -116,7 +116,17 @@ def pfx_filter(value):
     for key,val in value.items():
         if not isinstance(key, tuple):
             continue
-        table[key] = val
+        intf, ip_address = key
+        if '/' not in ip_address:
+            if is_ipv4(ip_address):
+                new_ip_address = "%s/32" % ip_address
+            elif is_ipv6(ip_address):
+                new_ip_address = "%s/128" % ip_address
+            else:
+                raise ValueError("'%s' is invalid ip address" % ip_address)
+            table[(intf, new_ip_address)] = val
+        else:
+            table[key] = val
     return table
 
 def ip_network(value):

src/sonic-config-engine/tests/data/pfx_filter/param_1.json

@@ -0,0 +1,12 @@
+{
+  "VLAN_INTERFACE": {
+    "Vlan1": {},
+    "Vlan1|1.1.1.1/32": {},
+    "Vlan2": {},
+    "Vlan2|2.2.2.2": {},
+    "Vlan3": {},
+    "Vlan3|fc00::1": {},
+    "Vlan4": {},
+    "Vlan4|fc00::2/64": {}
+  }
+}

src/sonic-config-engine/tests/data/pfx_filter/result_1.txt

@@ -0,0 +1,5 @@
+"Vlan1"="1.1.1.1/32"
+"Vlan2"="2.2.2.2/32"
+"Vlan3"="fc00::1/128"
+"Vlan4"="fc00::2/64"
+

src/sonic-config-engine/tests/data/pfx_filter/tmpl_1.txt.j2

@@ -0,0 +1,3 @@
+{% for intf, addr in VLAN_INTERFACE|pfx_filter %}
+"{{ intf }}"="{{ addr }}"
+{% endfor %}

src/sonic-config-engine/tests/test_cfggen_pfx_filter.py

@@ -0,0 +1,15 @@
+from unittest import TestCase
+import subprocess
+
+class TestPfxFilter(TestCase):
+    def test_comprehensive(self):
+        # Generate output
+        data_dir = "tests/data/pfx_filter"
+        cmd = "./sonic-cfggen -j %s/param_1.json -t %s/tmpl_1.txt.j2 > /tmp/result_1.txt" % (data_dir, data_dir)
+        subprocess.check_output(cmd, shell=True)
+        # Compare outputs
+        cmd = "diff -u tests/data/pfx_filter/result_1.txt /tmp/result_1.txt"
+        try:
+            res = subprocess.check_output(cmd, shell=True)
+        except subprocess.CalledProcessError as e:
+            assert False, "Wrong output. return code: %d, Diff: %s" % (e.returncode, e.output)