From 7d0ea7383d34834eb19e5453c0473d80fd98a212 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 | 12 +- 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, 196 insertions(+), 2 deletions(-) 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 index 5c8a4ed810..e880738811 100644 --- a/src/sonic-bgpcfgd/app/template.py +++ b/src/sonic-bgpcfgd/app/template.py @@ -4,6 +4,7 @@ from functools import partial import jinja2 import netaddr +from .log import log_err class TemplateFabric(object): """ Fabric for rendering jinja2 templates """ @@ -94,5 +95,14 @@ class TemplateFabric(object): 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 \ 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 0000000000..3eebd3951f --- /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 c0f61d8a95..8528f828c8 100755 --- a/src/sonic-config-engine/sonic-cfggen +++ b/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): 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 0000000000..ce6cd21886 --- /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 0000000000..d8ea7d3042 --- /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 0000000000..2612fe4a29 --- /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 0000000000..8ffd72907b --- /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)