From 17bf3993dd69474be46a4a646668b2181408dee6 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 26 Jun 2020 17:49:48 -0700 Subject: [PATCH 1/4] Fixed pfx_filter for bgpcfgd --- src/sonic-bgpcfgd/app/template.py | 12 +- src/sonic-bgpcfgd/tests/test_pfx_filter.py | 137 +++++++++++++++++++++ 2 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 src/sonic-bgpcfgd/tests/test_pfx_filter.py diff --git a/src/sonic-bgpcfgd/app/template.py b/src/sonic-bgpcfgd/app/template.py index 5c8a4ed810b..f6d87b05a5a 100644 --- a/src/sonic-bgpcfgd/app/template.py +++ b/src/sonic-bgpcfgd/app/template.py @@ -94,5 +94,15 @@ 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): + new_ip_address = "%s/32" % ip_address + elif TemplateFabric.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 \ 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 00000000000..14e3bf70f7d --- /dev/null +++ b/src/sonic-bgpcfgd/tests/test_pfx_filter.py @@ -0,0 +1,137 @@ +from app.template import TemplateFabric +from collections import OrderedDict + +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 + +def test_pfx_filter_pfx_exception(): + src = { + ('Loopback0', '23asef45'): {}, + } + try: + res = TemplateFabric.pfx_filter(src) + assert False, "Exception wasn't raised" + except ValueError as e: + assert str(e) == "'23asef45' is invalid ip address" From fb174b545000c7d831be8cadd8049a0e9955d47e Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Fri, 26 Jun 2020 18:22:47 -0700 Subject: [PATCH 2/4] Fix sonic-cfggen --- 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 +++++++++++++++ 5 files changed, 46 insertions(+), 1 deletion(-) 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-config-engine/sonic-cfggen b/src/sonic-config-engine/sonic-cfggen index c0f61d8a95f..8528f828c8a 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 00000000000..ce6cd21886a --- /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 00000000000..d8ea7d30422 --- /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 00000000000..2612fe4a293 --- /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 00000000000..a4bfa5e337c --- /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 = "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 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) From 15f55e7889f5e017861100d93b6dca0e3af8508b Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Mon, 29 Jun 2020 08:02:43 -0700 Subject: [PATCH 3/4] Ignore wrong ip address, don't crash bgpcfgd --- src/sonic-bgpcfgd/app/template.py | 8 ++++---- src/sonic-bgpcfgd/tests/test_pfx_filter.py | 16 +++++++++------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/sonic-bgpcfgd/app/template.py b/src/sonic-bgpcfgd/app/template.py index f6d87b05a5a..e8807388117 100644 --- a/src/sonic-bgpcfgd/app/template.py +++ b/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 """ @@ -97,12 +98,11 @@ def pfx_filter(value): intf, ip_address = key if '/' not in ip_address: if TemplateFabric.is_ipv4(ip_address): - new_ip_address = "%s/32" % ip_address + table[(intf, "%s/32" % ip_address)] = val elif TemplateFabric.is_ipv6(ip_address): - new_ip_address = "%s/128" % ip_address + table[(intf, "%s/128" % ip_address)] = val else: - raise ValueError("'%s' is invalid ip address" % ip_address) - table[(intf, new_ip_address)] = val + 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 index 14e3bf70f7d..3eebd3951f7 100644 --- a/src/sonic-bgpcfgd/tests/test_pfx_filter.py +++ b/src/sonic-bgpcfgd/tests/test_pfx_filter.py @@ -1,5 +1,7 @@ from app.template import TemplateFabric from collections import OrderedDict +import pytest + def test_pfx_filter_none(): res = TemplateFabric.pfx_filter(None) @@ -126,12 +128,12 @@ def test_pfx_filter_pfx_comprehensive(): res = TemplateFabric.pfx_filter(src) assert res == expected -def test_pfx_filter_pfx_exception(): +@pytest.fixture +def test_pfx_filter_wrong_ip(caplog): src = { - ('Loopback0', '23asef45'): {}, + ('Loopback0', 'wrong_ip'): {}, } - try: - res = TemplateFabric.pfx_filter(src) - assert False, "Exception wasn't raised" - except ValueError as e: - assert str(e) == "'23asef45' is invalid ip address" + res = TemplateFabric.pfx_filter(src) + assert "'wrong_ip' is invalid ip address" in caplog.text + assert isinstance(res, OrderedDict) and len(res) == 0 + From dfd57df3d351d5679618c88dfa4ed04fdaacc90d Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Tue, 30 Jun 2020 12:16:43 -0700 Subject: [PATCH 4/4] Fix test --- src/sonic-config-engine/tests/test_cfggen_pfx_filter.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py b/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py index a4bfa5e337c..8ffd72907b2 100644 --- a/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py +++ b/src/sonic-config-engine/tests/test_cfggen_pfx_filter.py @@ -4,11 +4,11 @@ class TestPfxFilter(TestCase): def test_comprehensive(self): # Generate output - data_dir = "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) + 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 data/pfx_filter/result_1.txt /tmp/result_1.txt" + 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: