Skip to content

Commit

Permalink
[GCU] Validate peer_group_range ip_range are correct (#2145)
Browse files Browse the repository at this point in the history
#### 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"}}]
```
  • Loading branch information
ghooo authored May 17, 2022
1 parent aa81b97 commit d7953d2
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 1 deletion.
25 changes: 24 additions & 1 deletion generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
74 changes: 74 additions & 0 deletions tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit d7953d2

Please sign in to comment.