diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index d004abdbcc..1979faa1d5 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -52,7 +52,17 @@ def apply(self, patch): self.logger.log_notice("Simulating the target full config after applying the patch.") target_config = self.patch_wrapper.simulate_patch(patch, old_config) - # Validate target config + # Validate target config does not have empty tables since they do not show up in ConfigDb + self.logger.log_notice("Validating target config does not have empty tables, " \ + "since they do not show up in ConfigDb.") + empty_tables = self.config_wrapper.get_empty_tables(target_config) + if empty_tables: # if there are empty tables + empty_tables_txt = ", ".join(empty_tables) + raise ValueError("Given patch is not valid because it will result in empty tables " \ + "which is not allowed in ConfigDb. " \ + f"Table{'s' if len(empty_tables) != 1 else ''}: {empty_tables_txt}") + + # Validate target config according to YANG models self.logger.log_notice("Validating target config according to YANG models.") if not(self.config_wrapper.validate_config_db_config(target_config)): raise ValueError(f"Given patch is not valid because it will result in an invalid config") diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 1f213e4371..3e775f1bb2 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -133,6 +133,14 @@ def crop_tables_without_yang(self, config_db_as_json): sy._cropConfigDB() return sy.jIn + + def get_empty_tables(self, config): + empty_tables = [] + for key in config.keys(): + if not(config[key]): + empty_tables.append(key) + return empty_tables + class DryRunConfigWrapper(ConfigWrapper): # TODO: implement DryRunConfigWrapper diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index ebc7c572ff..f781576fab 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -573,6 +573,35 @@ def _find_ref_paths(self, paths, config): refs.extend(self.path_addressing.find_ref_paths(path, config)) return refs +class NoEmptyTableMoveValidator: + """ + A class to validate that a move will not result in an empty table, because empty table do not show up in ConfigDB. + """ + def __init__(self, path_addressing): + self.path_addressing = path_addressing + + def validate(self, move, diff): + simulated_config = move.apply(diff.current_config) + op_path = move.path + + if op_path == "": # If updating whole file + tables_to_check = simulated_config.keys() + else: + tokens = self.path_addressing.get_path_tokens(op_path) + tables_to_check = [tokens[0]] + + return self._validate_tables(tables_to_check, simulated_config) + + def _validate_tables(self, tables, config): + for table in tables: + if not(self._validate_table(table, config)): + return False + return True + + def _validate_table(self, table, config): + # the only invalid case is if table exists and is empty + return table not in config or config[table] + class LowLevelMoveGenerator: """ A class to generate the low level moves i.e. moves corresponding to differences between current/target config @@ -969,7 +998,8 @@ def create(self, algorithm=Algorithm.DFS): FullConfigMoveValidator(self.config_wrapper), NoDependencyMoveValidator(self.path_addressing, self.config_wrapper), UniqueLanesMoveValidator(), - CreateOnlyMoveValidator(self.path_addressing) ] + CreateOnlyMoveValidator(self.path_addressing), + NoEmptyTableMoveValidator(self.path_addressing)] move_wrapper = MoveWrapper(move_generators, move_extenders, move_validators) diff --git a/tests/generic_config_updater/files/config_db_with_interface.json b/tests/generic_config_updater/files/config_db_with_interface.json index 2e1c488a4a..abd97d9875 100644 --- a/tests/generic_config_updater/files/config_db_with_interface.json +++ b/tests/generic_config_updater/files/config_db_with_interface.json @@ -4,7 +4,8 @@ "Ethernet8|10.0.0.1/30": { "family": "IPv4", "scope": "global" - } + }, + "Ethernet9": {} }, "PORT": { "Ethernet8": { @@ -15,6 +16,15 @@ "lanes": "65", "mtu": "9000", "speed": "25000" + }, + "Ethernet9": { + "admin_status": "up", + "alias": "eth9", + "description": "Ethernet9", + "fec": "rs", + "lanes": "6", + "mtu": "9000", + "speed": "25000" } } } diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index f201280062..ef6a219243 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/tests/generic_config_updater/generic_updater_test.py @@ -19,6 +19,13 @@ def test_apply__invalid_patch_updating_tables_without_yang_models__failure(self) # Act and assert self.assertRaises(ValueError, patch_applier.apply, Files.MULTI_OPERATION_CONFIG_DB_PATCH) + def test_apply__invalid_patch_producing_empty_tables__failure(self): + # Arrange + patch_applier = self.__create_patch_applier(valid_patch_does_not_produce_empty_tables=False) + + # Act and assert + self.assertRaises(ValueError, patch_applier.apply, Files.MULTI_OPERATION_CONFIG_DB_PATCH) + def test_apply__invalid_config_db__failure(self): # Arrange patch_applier = self.__create_patch_applier(valid_config_db=False) @@ -58,12 +65,16 @@ def __create_patch_applier(self, changes=None, valid_patch_only_tables_with_yang_models=True, valid_config_db=True, + valid_patch_does_not_produce_empty_tables=True, verified_same_config=True): config_wrapper = Mock() config_wrapper.get_config_db_as_json.side_effect = \ [Files.CONFIG_DB_AS_JSON, Files.CONFIG_DB_AFTER_MULTI_PATCH] config_wrapper.validate_config_db_config.side_effect = \ create_side_effect_dict({(str(Files.CONFIG_DB_AFTER_MULTI_PATCH),): valid_config_db}) + empty_tables = [] if valid_patch_does_not_produce_empty_tables else ["AnyTable"] + config_wrapper.get_empty_tables.side_effect = \ + create_side_effect_dict({(str(Files.CONFIG_DB_AFTER_MULTI_PATCH),): empty_tables}) patch_wrapper = Mock() patch_wrapper.validate_config_db_patch_has_yang_models.side_effect = \ diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index f69ec08030..842e71baaa 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -148,6 +148,39 @@ def test_crop_tables_without_yang__returns_cropped_config_db_as_json(self): # Assert self.assertDictEqual(expected, actual) + def test_get_empty_tables__no_empty_tables__returns_no_tables(self): + # Arrange + config_wrapper = gu_common.ConfigWrapper() + config = {"any_table": {"key": "value"}} + + # Act + empty_tables = config_wrapper.get_empty_tables(config) + + # Assert + self.assertCountEqual([], empty_tables) + + def test_get_empty_tables__single_empty_table__returns_one_table(self): + # Arrange + config_wrapper = gu_common.ConfigWrapper() + config = {"any_table": {"key": "value"}, "another_table":{}} + + # Act + empty_tables = config_wrapper.get_empty_tables(config) + + # Assert + self.assertCountEqual(["another_table"], empty_tables) + + def test_get_empty_tables__multiple_empty_tables__returns_multiple_tables(self): + # Arrange + config_wrapper = gu_common.ConfigWrapper() + config = {"any_table": {"key": "value"}, "another_table":{}, "yet_another_table":{}} + + # Act + empty_tables = config_wrapper.get_empty_tables(config) + + # Assert + self.assertCountEqual(["another_table", "yet_another_table"], empty_tables) + class TestPatchWrapper(unittest.TestCase): def setUp(self): self.config_wrapper_mock = gu_common.ConfigWrapper() diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 4da9fb901b..c51733abe7 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1008,6 +1008,121 @@ def test_validate__replace_whole_config_item_same_ref_same__true(self): def prepare_config(self, config, patch): return patch.apply(config) +class TestNoEmptyTableMoveValidator(unittest.TestCase): + def setUp(self): + path_addressing = ps.PathAddressing() + self.validator = ps.NoEmptyTableMoveValidator(path_addressing) + + def test_validate__no_changes__success(self): + # Arrange + current_config = {"some_table":{"key1":"value1", "key2":"value2"}} + target_config = {"some_table":{"key1":"value1", "key2":"value22"}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key1"], ["some_table", "key1"]) + + # Act and assert + self.assertTrue(self.validator.validate(move, diff)) + + def test_validate__change_but_no_empty_table__success(self): + # Arrange + current_config = {"some_table":{"key1":"value1", "key2":"value2"}} + target_config = {"some_table":{"key1":"value1", "key2":"value22"}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table", "key2"], ["some_table", "key2"]) + + # Act and assert + self.assertTrue(self.validator.validate(move, diff)) + + def test_validate__single_empty_table__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1", "key2":"value2"}} + target_config = {"some_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, ["some_table"], ["some_table"]) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__whole_config_replace_single_empty_table__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1", "key2":"value2"}} + target_config = {"some_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, [], []) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__whole_config_replace_mix_of_empty_and_non_empty__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"some_table":{"key1":"value1"}, "other_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, [], []) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__whole_config_multiple_empty_tables__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"some_table":{}, "other_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REPLACE, [], []) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__remove_key_empties_a_table__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"some_table":{"key1":"value1"}, "other_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], []) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__remove_key_but_table_has_other_keys__success(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2", "key3":"value3"}} + target_config = {"some_table":{"key1":"value1"}, "other_table":{"key3":"value3"}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table", "key2"], []) + + # Act and assert + self.assertTrue(self.validator.validate(move, diff)) + + def test_validate__remove_whole_table__success(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"some_table":{"key1":"value1"}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.REMOVE, ["other_table"], []) + + # Act and assert + self.assertTrue(self.validator.validate(move, diff)) + + def test_validate__add_empty_table__failure(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"new_table":{}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"]) + + # Act and assert + self.assertFalse(self.validator.validate(move, diff)) + + def test_validate__add_non_empty_table__success(self): + # Arrange + current_config = {"some_table":{"key1":"value1"}, "other_table":{"key2":"value2"}} + target_config = {"new_table":{"key3":"value3"}} + diff = ps.Diff(current_config, target_config) + move = ps.JsonMove(diff, OperationType.ADD, ["new_table"], ["new_table"]) + + # Act and assert + self.assertTrue(self.validator.validate(move, diff)) + class TestLowLevelMoveGenerator(unittest.TestCase): def setUp(self): path_addressing = PathAddressing() @@ -1566,7 +1681,8 @@ def verify(self, algo, algo_class): ps.FullConfigMoveValidator, ps.NoDependencyMoveValidator, ps.UniqueLanesMoveValidator, - ps.CreateOnlyMoveValidator] + ps.CreateOnlyMoveValidator, + ps.NoEmptyTableMoveValidator] # Act sorter = factory.create(algo) @@ -1712,13 +1828,34 @@ def test_sort__modify_items_with_dependencies_using_must__success(self): {"op":"replace", "path":"/CRM/Config/acl_counter_low_threshold", "value":"60"}], cc_ops=[{"op":"replace", "path":"", "value":Files.CONFIG_DB_WITH_CRM}]) + def test_sort__modify_items_result_in_empty_table__failure(self): + # Emptying a table + self.assertRaises(GenericConfigUpdaterError, + self.verify, + cc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}], + tc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}, + {"op":"replace", "path":"/ACL_TABLE", "value":{}}]) + # Adding an empty table + self.assertRaises(GenericConfigUpdaterError, + self.verify, + cc_ops=[{"op":"replace", "path":"", "value":Files.ANY_CONFIG_DB}], + tc_ops=[{"op":"replace", "path":"", "value":Files.ANY_CONFIG_DB}, + {"op":"add", "path":"/VLAN", "value":{}}]) + # Emptying multiple tables + self.assertRaises(GenericConfigUpdaterError, + self.verify, + cc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}], + tc_ops=[{"op":"replace", "path":"", "value":Files.SIMPLE_CONFIG_DB_INC_DEPS}, + {"op":"replace", "path":"/ACL_TABLE", "value":{}}, + {"op":"replace", "path":"/PORT", "value":{}}]) + def verify(self, cc_ops=[], tc_ops=[]): # Arrange config_wrapper=ConfigWrapper() target_config=jsonpatch.JsonPatch(tc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON) current_config=jsonpatch.JsonPatch(cc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON) patch=jsonpatch.make_patch(current_config, target_config) - + # Act actual = self.create_patch_sorter(current_config).sort(patch)