[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 <[email protected]>

Author: pavel-shirshov

src/sonic-bgpcfgd/app/template.py

@@ -4,6 +4,7 @@
 import jinja2
 import netaddr
 
+from .log import log_err
 
 class TemplateFabric(object):
     """ Fabric for rendering jinja2 templates """
@@ -94,5 +95,14 @@ 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 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

@@ -113,7 +113,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)