diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index b53024e17380..b795421064f1 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -372,40 +372,58 @@ def validate(self, move, diff): class CreateOnlyMoveValidator: """ - A class to validate create-only fields are only added/removed but never replaced. - Parents of create-only fields are also only added/removed but never replaced when they contain - a modified create-only field. + A class to validate create-only fields are only created, but never modified/updated. In other words: + - Field cannot be replaced. + - Field cannot be added, only if the parent is added. + - Field cannot be deleted, only if the parent is deleted. """ def __init__(self, path_addressing): self.path_addressing = path_addressing - def validate(self, move, diff): - if move.op_type != OperationType.REPLACE: - return True + # TODO: create-only fields are hard-coded for now, it should be moved to YANG models + # Each pattern consist of a list of tokens. Token matching starts from the root level of the config. + # Each token is either a specific key or '*' to match all keys. + self.create_only_patterns = [ + ["PORT", "*", "lanes"], + ["LOOPBACK_INTERFACE", "*", "vrf_name"], + ] - # The 'create-only' field needs to be common between current and simulated anyway but different. - # This means it is enough to just get the paths from current_config, paths that are not common can be ignored. - paths = self._get_create_only_paths(diff.current_config) + 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))) for path in paths: tokens = self.path_addressing.get_path_tokens(path) if self._value_exist_but_different(tokens, diff.current_config, simulated_config): return False + if self._value_added_but_parent_exist(tokens, diff.current_config, simulated_config): + return False + if self._value_removed_but_parent_remain(tokens, diff.current_config, simulated_config): + return False return True - # TODO: create-only fields are hard-coded for now, it should be moved to YANG models def _get_create_only_paths(self, config): - if "PORT" not in config: + for pattern in self.create_only_patterns: + for create_only_path in self._get_create_only_path_recursive(config, pattern, [], 0): + yield create_only_path + + def _get_create_only_path_recursive(self, config, pattern_tokens, matching_tokens, idx): + if idx == len(pattern_tokens): + yield '/' + '/'.join(matching_tokens) return - ports = config["PORT"] + matching_keys = [] + if pattern_tokens[idx] == "*": + matching_keys = config.keys() + elif pattern_tokens[idx] in config: + matching_keys = [pattern_tokens[idx]] - for port in ports: - attrs = ports[port] - if "lanes" in attrs: - yield f"/PORT/{port}/lanes" + for key in matching_keys: + matching_tokens.append(key) + for create_only_path in self._get_create_only_path_recursive(config[key], pattern_tokens, matching_tokens, idx+1): + yield create_only_path + matching_tokens.pop() def _value_exist_but_different(self, tokens, current_config_ptr, simulated_config_ptr): for token in tokens: @@ -422,6 +440,46 @@ def _value_exist_but_different(self, tokens, current_config_ptr, simulated_confi return current_config_ptr != simulated_config_ptr + def _value_added_but_parent_exist(self, tokens, current_config_ptr, simulated_config_ptr): + # if value is not added, return false + if not self._exist_only_in_first(tokens, simulated_config_ptr, current_config_ptr): + return False + + # if parent is added, return false + if self._exist_only_in_first(tokens[:-1], simulated_config_ptr, current_config_ptr): + return False + + # otherwise parent exist and value is added + return True + + def _value_removed_but_parent_remain(self, tokens, current_config_ptr, simulated_config_ptr): + # if value is not removed, return false + if not self._exist_only_in_first(tokens, current_config_ptr, simulated_config_ptr): + return False + + # if parent is removed, return false + if self._exist_only_in_first(tokens[:-1], current_config_ptr, simulated_config_ptr): + return False + + # otherwise parent remained and value is removed + 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 + class NoDependencyMoveValidator: """ A class to validate that the modified configs do not have dependency on each other. This should prevent diff --git a/tests/generic_config_updater/files/config_db_with_loopback_interfaces.json b/tests/generic_config_updater/files/config_db_with_loopback_interfaces.json new file mode 100644 index 000000000000..2c1724a4cdd7 --- /dev/null +++ b/tests/generic_config_updater/files/config_db_with_loopback_interfaces.json @@ -0,0 +1,16 @@ +{ + "LOOPBACK_INTERFACE": { + "Loopback0": {}, + "Loopback0|10.1.0.32/32": {}, + "Loopback0|1100:1::32/128": {}, + "Loopback1": { + "vrf_name": "Vrf_02" + }, + "Loopback1|20.2.0.32/32": {}, + "Loopback1|2200:2::32/128": {} + }, + "VRF": { + "Vrf_01": {}, + "Vrf_02": {} + } +} \ 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 b1764a8dfc68..ef809ef929c1 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -784,13 +784,6 @@ def setUp(self): self.validator = ps.CreateOnlyMoveValidator(ps.PathAddressing()) self.any_diff = ps.Diff({}, {}) - def test_validate__non_replace_operation__success(self): - # Assert - self.assertTrue(self.validator.validate( \ - ps.JsonMove(self.any_diff, OperationType.ADD, [], []), self.any_diff)) - self.assertTrue(self.validator.validate( \ - ps.JsonMove(self.any_diff, OperationType.REMOVE, [], []), self.any_diff)) - def test_validate__no_create_only_field__success(self): current_config = {"PORT": {}} target_config = {"PORT": {}, "ACL_TABLE": {}} @@ -833,7 +826,7 @@ def test_validate__different_create_only_field_updating_grandparent__failure(sel ["PORT"], False) - def test_validate__same_create_only_field_directly_updated__failure(self): + def test_validate__same_create_only_field_directly_updated__success(self): current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}} target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} self.verify_diff(current_config, @@ -841,7 +834,7 @@ def test_validate__same_create_only_field_directly_updated__failure(self): ["PORT", "Ethernet0", "lanes"], ["PORT", "Ethernet0", "lanes"]) - def test_validate__same_create_only_field_updating_parent__failure(self): + def test_validate__same_create_only_field_updating_parent__success(self): current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}} target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} self.verify_diff(current_config, @@ -849,7 +842,7 @@ def test_validate__same_create_only_field_updating_parent__failure(self): ["PORT", "Ethernet0"], ["PORT", "Ethernet0"]) - def test_validate__same_create_only_field_updating_grandparent__failure(self): + def test_validate__same_create_only_field_updating_grandparent__success(self): current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}} target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} self.verify_diff(current_config, @@ -857,6 +850,62 @@ def test_validate__same_create_only_field_updating_grandparent__failure(self): ["PORT"], ["PORT"]) + def test_validate__added_create_only_field_parent_exist__failure(self): + current_config = {"PORT": {"Ethernet0":{}}} + target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} + self.verify_diff(current_config, + target_config, + ["PORT"], + ["PORT"], + expected=False) + + def test_validate__added_create_only_field_parent_doesnot_exist__success(self): + current_config = {"PORT": {}} + target_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} + self.verify_diff(current_config, + target_config, + ["PORT"], + ["PORT"]) + + def test_validate__removed_create_only_field_parent_remain__failure(self): + current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} + target_config = {"PORT": {"Ethernet0":{}}} + self.verify_diff(current_config, + target_config, + ["PORT"], + ["PORT"], + expected=False) + + def test_validate__removed_create_only_field_parent_doesnot_remain__success(self): + current_config = {"PORT": {"Ethernet0":{"lanes":"65"}}, "ACL_TABLE": {}} + target_config = {"PORT": {}} + self.verify_diff(current_config, + target_config, + ["PORT"], + ["PORT"]) + + def test_hard_coded_create_only_paths(self): + config = { + "PORT": { + "Ethernet0":{"lanes":"65"}, + "Ethernet1":{}, + "Ethernet2":{"lanes":"66,67"} + }, + "LOOPBACK_INTERFACE": { + "Loopback0":{"vrf_name":"vrf0"}, + "Loopback1":{}, + "Loopback2":{"vrf_name":"vrf1"}, + }} + expected = [ + "/PORT/Ethernet0/lanes", + "/PORT/Ethernet2/lanes", + "/LOOPBACK_INTERFACE/Loopback0/vrf_name", + "/LOOPBACK_INTERFACE/Loopback2/vrf_name" + ] + actual = self.validator._get_create_only_paths(config) + + self.assertCountEqual(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 [] @@ -1741,6 +1790,26 @@ def test_sort__replacing_create_only_field__success(self): # Assert self.assertNotEqual(None, actual) + def test_sort__adding_create_only_field__success(self): + # Arrange + patch = jsonpatch.JsonPatch([{"op":"add", "path": "/LOOPBACK_INTERFACE/Loopback0/vrf_name", "value":"Vrf_01"}]) + + # Act + actual = self.create_patch_sorter(Files.CONFIG_DB_WITH_LOOPBACK_INTERFACES).sort(patch) + + # Assert + self.assertNotEqual(None, actual) + + def test_sort__removing_create_only_field__success(self): + # Arrange + patch = jsonpatch.JsonPatch([{"op":"remove", "path": "/LOOPBACK_INTERFACE/Loopback1/vrf_name"}]) + + # Act + actual = self.create_patch_sorter(Files.CONFIG_DB_WITH_LOOPBACK_INTERFACES).sort(patch) + + # Assert + self.assertNotEqual(None, actual) + def test_sort__inter_dependency_within_same_table__success(self): # Arrange patch = jsonpatch.JsonPatch([{"op":"add", "path":"/VLAN_INTERFACE", "value": {