From 4c600e8d765096f85ce296d1dccf15a9fe040fde Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Mon, 16 May 2022 21:27:19 -0700 Subject: [PATCH] [GCU] Validate peer_group_range ip_range are correct (#2145) #### What I did Fixes #2119 #### Previous command output (if the output of a command-line utility has changed) Sorting output: ``` Patch Applier: Applying 4 changes in order: Patch Applier: * [{"op": "remove", "path": "/BGP_PEER_RANGE/BGPSLBPassiveV6"}] Patch Applier: * [{"op": "add", "path": "/BGP_PEER_RANGE/BGPVac", "value": {"ip_range": ["192.168.0.0/21"], "name": "BGPVac", "src_address": "10.1.0.32"}}] Patch Applier: * [{"op": "remove", "path": "/BGP_PEER_RANGE/BGPSLBPassive"}] Patch Applier: * [{"op": "add", "path": "/BGP_PEER_RANGE/BGPSLBPassive", "value": {"ip_range": ["10.255.0.0/25"], "name": "BGPSLBPassive", "src_address": "10.1.0.32"}}] ``` #### New command output (if the output of a command-line utility has changed) Sorting output: ``` Patch Applier: Applying 4 changes in order: Patch Applier: * [{"op": "remove", "path": "/BGP_PEER_RANGE/BGPSLBPassiveV6"}] Patch Applier: * [{"op": "remove", "path": "/BGP_PEER_RANGE"}] Patch Applier: * [{"op": "add", "path": "/BGP_PEER_RANGE", "value": {"BGPSLBPassive": {"ip_range": ["10.255.0.0/25"], "name": "BGPSLBPassive", "src_address": "10.1.0.32"}}}] Patch Applier: * [{"op": "add", "path": "/BGP_PEER_RANGE/BGPVac", "value": {"ip_range": ["192.168.0.0/21"], "name": "BGPVac", "src_address": "10.1.0.32"}}] ``` --- generic_config_updater/gu_common.py | 25 ++++++- .../generic_config_updater/gu_common_test.py | 74 +++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 356bff3435..3a7e9346fc 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -118,10 +118,33 @@ def validate_config_db_config(self, config_db_as_json): sy.loadData(tmp_config_db_as_json) sy.validate_data_tree() - return True, None + + # TODO: modularize custom validations better or move directly to sonic-yang module + return self.validate_bgp_peer_group(config_db_as_json) except sonic_yang.SonicYangException as ex: return False, ex + def validate_bgp_peer_group(self, config_db): + if "BGP_PEER_RANGE" not in config_db: + return True, None + + visited = {} + table = config_db["BGP_PEER_RANGE"] + for peer_group_name in table: + peer_group = table[peer_group_name] + if "ip_range" not in peer_group: + continue + + # TODO: convert string to IpAddress object for better handling of IPs + # TODO: validate range intersection + ip_range = peer_group["ip_range"] + for ip in ip_range: + if ip in visited: + return False, f"{ip} is duplicated in BGP_PEER_RANGE: {set([peer_group_name, visited[ip]])}" + visited[ip] = peer_group_name + + return True, None + def crop_tables_without_yang(self, config_db_as_json): sy = self.create_sonic_yang_with_loaded_models() diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index e3a8a3bde0..90702d1592 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -186,6 +186,80 @@ def test_validate_config_db_config__invalid_config__returns_false(self): self.assertEqual(expected, actual) self.assertIsNotNone(error) + def test_validate_bgp_peer_group__valid_non_intersecting_ip_ranges__returns_true(self): + # Arrange + config_wrapper = gu_common.ConfigWrapper() + config = { + "BGP_PEER_RANGE": + { + "BGPSLBPassive": { + "ip_range": ["1.1.1.1/31", "10.10.10.10/16", "100.100.100.100/24"] + }, + "BgpVac": { + "ip_range": ["2.2.2.2/31", "20.20.20.20/16", "200.200.200.200/24"] + } + } + } + + # Act + actual, error = config_wrapper.validate_bgp_peer_group(config) + + # Assert + self.assertTrue(actual) + self.assertIsNone(error) + + def test_validate_bgp_peer_group__same_ip_prefix__return_false(self): + # duplicate v4 within same ip_range + self.check_validate_bgp_peer_group( + ["1.1.1.1/16", "1.1.1.1/16"], + duplicated_ip="1.1.1.1/16") + # duplicate v4 within different ip_ranges + self.check_validate_bgp_peer_group( + ["1.1.1.1/16"], + ["1.1.1.1/16"], + duplicated_ip="1.1.1.1/16") + # duplicate v4 within different ip_ranges, but many ips + self.check_validate_bgp_peer_group( + ["1.1.1.1/16", "1.1.1.1/31", "10.10.10.10/16", "100.100.100.100/24"], + ["2.2.2.2/31", "20.20.20.20/16", "200.200.200.200/24", "1.1.1.1/16"], + duplicated_ip="1.1.1.1/16") + # duplicate v6 within same ip_range + self.check_validate_bgp_peer_group( + ["fc00:1::32/16", "fc00:1::32/16"], + duplicated_ip="fc00:1::32/16") + # duplicate v6 within different ip_ranges + self.check_validate_bgp_peer_group( + ["fc00:1::32/16"], + ["fc00:1::32/16"], + duplicated_ip="fc00:1::32/16") + # duplicate v6 within different ip_ranges, but many ips + self.check_validate_bgp_peer_group( + ["fc00:1::32/16", "fc00:1::32/31", "10:1::1/16", "100:1::1/24"], + ["2:1::1/31", "20:1::1/16", "200:1::1/24", "fc00:1::32/16"], + duplicated_ip="fc00:1::32/16") + + def check_validate_bgp_peer_group(self, ip_range, other_ip_range=[], duplicated_ip=None): + # Arrange + config_wrapper = gu_common.ConfigWrapper() + config = { + "BGP_PEER_RANGE": + { + "BGPSLBPassive": { + "ip_range": ip_range + }, + "BgpVac": { + "ip_range": other_ip_range + }, + } + } + + # Act + actual, error = config_wrapper.validate_bgp_peer_group(config) + + # Assert + self.assertFalse(actual) + self.assertTrue(duplicated_ip in error) + def test_crop_tables_without_yang__returns_cropped_config_db_as_json(self): # Arrange config_wrapper = gu_common.ConfigWrapper()