From 7d840bac3fd5641b22c31bcf4cb470694e3c53b0 Mon Sep 17 00:00:00 2001 From: pavel-shirshov Date: Thu, 2 Jul 2020 00:22:58 -0700 Subject: [PATCH] [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 --- src/sonic-bgpcfgd/app/template.py | 108 ++++++++++++++ src/sonic-bgpcfgd/tests/test_pfx_filter.py | 139 ++++++++++++++++++ src/sonic-config-engine/sonic-cfggen | 12 +- .../tests/data/pfx_filter/param_1.json | 12 ++ .../tests/data/pfx_filter/result_1.txt | 5 + .../tests/data/pfx_filter/tmpl_1.txt.j2 | 3 + .../tests/test_cfggen_pfx_filter.py | 15 ++ 7 files changed, 293 insertions(+), 1 deletion(-) create mode 100644 src/sonic-bgpcfgd/app/template.py create mode 100644 src/sonic-bgpcfgd/tests/test_pfx_filter.py create mode 100644 src/sonic-config-engine/tests/data/pfx_filter/param_1.json create mode 100644 src/sonic-config-engine/tests/data/pfx_filter/result_1.txt create mode 100644 src/sonic-config-engine/tests/data/pfx_filter/tmpl_1.txt.j2 create mode 100644 src/sonic-config-engine/tests/test_cfggen_pfx_filter.py diff --git a/src/sonic-bgpcfgd/app/template.py b/src/sonic-bgpcfgd/app/template.py new file mode 100644 index 000000000000..e88073881171 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/src/sonic-bgpcfgd/tests/test_pfx_filter.py b/src/sonic-bgpcfgd/tests/test_pfx_filter.py new file mode 100644 index 000000000000..3eebd3951f7b --- /dev/null +++ b/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 + diff --git a/src/sonic-config-engine/sonic-cfggen b/src/sonic-config-engine/sonic-cfggen index 1bc5920b584e..c449bb1d17b4 100755 --- a/src/sonic-config-engine/sonic-cfggen +++ b/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): diff --git a/src/sonic-config-engine/tests/data/pfx_filter/param_1.json b/src/sonic-config-engine/tests/data/pfx_filter/param_1.json new file mode 100644 index 000000000000..ce6cd21886a2 --- /dev/null +++ b/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": {} + } +} diff --git a/src/sonic-config-engine/tests/data/pfx_filter/result_1.txt b/src/sonic-config-engine/tests/data/pfx_filter/result_1.txt new file mode 100644 index 000000000000..d8ea7d304222 --- /dev/null +++ b/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" + diff --git a/src/sonic-config-engine/tests/data/pfx_filter/tmpl_1.txt.j2 b/src/sonic-config-engine/tests/data/pfx_filter/tmpl_1.txt.j2 new file mode 100644 index 000000000000..2612fe4a2936 --- /dev/null +++ b/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 %} diff --git a/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py b/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py new file mode 100644 index 000000000000..8ffd72907b21 --- /dev/null +++ b/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)