diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index daf5464245..f7a5b37efe 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -393,11 +393,23 @@ def __init__(self, path_addressing): self.create_only_patterns = [ ["PORT", "*", "lanes"], ["LOOPBACK_INTERFACE", "*", "vrf_name"], + ["BGP_NEIGHBOR", "*", "holdtime"], + ["BGP_NEIGHBOR", "*", "keepalive"], + ["BGP_NEIGHBOR", "*", "name"], + ["BGP_NEIGHBOR", "*", "asn"], + ["BGP_NEIGHBOR", "*", "local_addr"], + ["BGP_NEIGHBOR", "*", "nhopself"], + ["BGP_NEIGHBOR", "*", "rrclient"], ] def validate(self, move, diff): simulated_config = move.apply(diff.current_config) - paths = set(list(self._get_create_only_paths(diff.current_config)) + list(self._get_create_only_paths(simulated_config))) + # get create-only paths from current config, simulated config and also target config + # simulated config is the result of the move + # target config is the final config + paths = set(list(self._get_create_only_paths(diff.current_config)) + + list(self._get_create_only_paths(simulated_config)) + + list(self._get_create_only_paths(diff.target_config))) for path in paths: tokens = self.path_addressing.get_path_tokens(path) @@ -408,8 +420,28 @@ def validate(self, move, diff): if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config): return False + # if parent of create-only field is added, create-only field should be the same as target + # i.e. if field is deleted in target, it should be deleted in the move, or + # if field is present in target, it should be present in the move + if self._parent_added_child_not_as_target(tokens, diff.current_config, simulated_config, diff.target_config): + return False + return True + def _parent_added_child_not_as_target(self, tokens, current_config, simulated_config, target_config): + # if parent is not added, return false + if not self._exist_only_in_first(tokens[:-1], simulated_config, current_config): + return False + + child_path = self.path_addressing.create_path(tokens) + + # if child is in target, check if child is not in simulated + if self.path_addressing.has_path(target_config, child_path): + return not self.path_addressing.has_path(simulated_config, child_path) + else: + # if child is not in target, check if child is in simulated + return self.path_addressing.has_path(simulated_config, child_path) + def _get_create_only_paths(self, config): for pattern in self.create_only_patterns: for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0): @@ -472,20 +504,9 @@ def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated return True def _exist_only_in_first(self, tokens, first_config_ptr, second_config_ptr): - for token in tokens: - mod_token = int(token) if isinstance(first_config_ptr, list) else token - - if mod_token not in second_config_ptr: - return True - - if mod_token not in first_config_ptr: - return False - - first_config_ptr = first_config_ptr[mod_token] - second_config_ptr = second_config_ptr[mod_token] - - # tokens exist in both - return False + path = self.path_addressing.create_path(tokens) + return self.path_addressing.has_path(first_config_ptr, path) and \ + not self.path_addressing.has_path(second_config_ptr, path) class NoDependencyMoveValidator: """ diff --git a/tests/generic_config_updater/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index ef863170de..11e061d091 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -2894,5 +2894,85 @@ } ] ] + }, + "ADDING_BGP_NEIGHBORS": { + "current_config": { + "BGP_NEIGHBOR": { + "10.0.0.57": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.56", + "name": "ARISTA01T1", + "nhopself": "0", + "rrclient": "0" + } + } + }, + "patch": [ + { + "op": "add", + "path": "/BGP_NEIGHBOR/10.0.0.59", + "value": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.58", + "name": "ARISTA02T1", + "nhopself": "0", + "rrclient": "0" + } + }, + { + "op": "add", + "path": "/BGP_NEIGHBOR/10.0.0.61", + "value": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.60", + "name": "ARISTA03T1", + "nhopself": "0", + "rrclient": "0" + } + } + ], + "expected_changes": [ + [ + { + "op": "add", + "path": "/BGP_NEIGHBOR/10.0.0.59", + "value": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.58", + "name": "ARISTA02T1", + "nhopself": "0", + "rrclient": "0" + } + } + ], + [ + { + "op": "add", + "path": "/BGP_NEIGHBOR/10.0.0.61", + "value": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.60", + "name": "ARISTA03T1", + "nhopself": "0", + "rrclient": "0" + } + } + ] + ] } } \ No newline at end of file diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 29c1b6562e..41805b698f 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -884,6 +884,31 @@ def test_validate__removed_create_only_field_parent_doesnot_remain__success(self ["PORT"], ["PORT"]) + def test_validate__parent_added_with_all_create_only_field_that_target_has__success(self): + added_parent_value = { + "admin_status": "up", + "asn": "64600", # <== create-only field + "holdtime": "50", # <== create-only field + } + self.verify_parent_adding(added_parent_value, True) + + def test_validate__parent_added_with_create_only_field_but_target_does_not_have_the_field__failure(self): + added_parent_value = { + "admin_status": "up", + "asn": "64600", # <== create-only field + "holdtime": "50", # <== create-only field + "rrclient": "1", # <== create-only field but not in target-config + } + self.verify_parent_adding(added_parent_value, False) + + def test_validate__parent_added_without_create_only_field_but_target_have_the_field__failure(self): + added_parent_value = { + "admin_status": "up", + "asn": "64600", # <== create-only field + # Not adding: "holdtime": "50" + } + self.verify_parent_adding(added_parent_value, False) + def test_hard_coded_create_only_paths(self): config = { "PORT": { @@ -895,17 +920,59 @@ def test_hard_coded_create_only_paths(self): "Loopback0":{"vrf_name":"vrf0"}, "Loopback1":{}, "Loopback2":{"vrf_name":"vrf1"}, - }} + }, + "BGP_NEIGHBOR": { + "10.0.0.57": { + "admin_status": "up", + "asn": "64600", + "holdtime": "10", + "keepalive": "3", + "local_addr": "10.0.0.56", + "name": "ARISTA01T1", + "nhopself": "0", + "rrclient": "0" + } + } + } expected = [ "/PORT/Ethernet0/lanes", "/PORT/Ethernet2/lanes", "/LOOPBACK_INTERFACE/Loopback0/vrf_name", - "/LOOPBACK_INTERFACE/Loopback2/vrf_name" + "/LOOPBACK_INTERFACE/Loopback2/vrf_name", + "/BGP_NEIGHBOR/10.0.0.57/asn", + "/BGP_NEIGHBOR/10.0.0.57/holdtime", + "/BGP_NEIGHBOR/10.0.0.57/keepalive", + "/BGP_NEIGHBOR/10.0.0.57/local_addr", + "/BGP_NEIGHBOR/10.0.0.57/name", + "/BGP_NEIGHBOR/10.0.0.57/nhopself", + "/BGP_NEIGHBOR/10.0.0.57/rrclient", ] + actual = self.validator._get_create_only_paths(config) self.assertCountEqual(expected, actual) + def verify_parent_adding(self, added_parent_value, expected): + current_config = { + "BGP_NEIGHBOR": {} + } + + target_config = { + "BGP_NEIGHBOR": { + "10.0.0.57": { + "admin_status": "up", + "asn": "64600", + "holdtime": "50", + } + } + } + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove.from_operation({"op":"add", "path":"/BGP_NEIGHBOR/10.0.0.57", "value": added_parent_value}) + + actual = self.validator.validate(move, diff) + + self.assertEqual(expected, actual) + def verify_diff(self, current_config, target_config, current_config_tokens=None, target_config_tokens=None, expected=True): # Arrange current_config_tokens = current_config_tokens if current_config_tokens else []