diff --git a/config/aaa.py b/config/aaa.py index 18b467a804..ffe974d4f6 100644 --- a/config/aaa.py +++ b/config/aaa.py @@ -2,8 +2,11 @@ import ipaddress import re from swsscommon.swsscommon import ConfigDBConnector +from .validated_config_db_connector import ValidatedConfigDBConnector +from jsonpatch import JsonPatchConflict import utilities_common.cli as clicommon +ADHOC_VALIDATION = True RADIUS_MAXSERVERS = 8 RADIUS_PASSKEY_MAX_LEN = 65 VALID_CHARS_MSG = "Valid chars are ASCII printable except SPACE, '#', and ','" @@ -13,19 +16,27 @@ def is_secret(secret): def add_table_kv(table, entry, key, val): - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() - config_db.mod_entry(table, entry, {key:val}) + try: + config_db.mod_entry(table, entry, {key:val}) + except ValueError as e: + ctx = click.get_current_context() + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) def del_table_key(table, entry, key): - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() data = config_db.get_entry(table, entry) if data: if key in data: del data[key] - config_db.set_entry(table, entry, data) + try: + config_db.set_entry(table, entry, data) + except (ValueError, JsonPatchConflict) as e: + ctx = click.get_current_context() + ctx.fail("Invalid ConfigDB. Error: {}".format(e)) @click.group() def aaa(): @@ -246,11 +257,12 @@ def passkey(ctx, secret): @click.option('-m', '--use-mgmt-vrf', help="Management vrf, default is no vrf", is_flag=True) def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf): """Specify a TACACS+ server""" - if not clicommon.is_ipaddress(address): - click.echo('Invalid ip address') - return + if ADHOC_VALIDATION: + if not clicommon.is_ipaddress(address): + click.echo('Invalid ip address') # TODO: MISSING CONSTRAINT IN YANG MODEL + return - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() old_data = config_db.get_entry('TACPLUS_SERVER', address) if old_data != {}: @@ -268,7 +280,11 @@ def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf): data['passkey'] = key if use_mgmt_vrf : data['vrf'] = "mgmt" - config_db.set_entry('TACPLUS_SERVER', address, data) + try: + config_db.set_entry('TACPLUS_SERVER', address, data) + except ValueError as e: + ctx = click.get_current_context() + ctx.fail("Invalid ip address. Error: {}".format(e)) tacacs.add_command(add) @@ -278,13 +294,18 @@ def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf): @click.argument('address', metavar='') def delete(address): """Delete a TACACS+ server""" - if not clicommon.is_ipaddress(address): - click.echo('Invalid ip address') - return + if ADHOC_VALIDATION: + if not clicommon.is_ipaddress(address): + click.echo('Invalid ip address') + return - config_db = ConfigDBConnector() + config_db = ValidatedConfigDBConnector(ConfigDBConnector()) config_db.connect() - config_db.set_entry('TACPLUS_SERVER', address, None) + try: + config_db.set_entry('TACPLUS_SERVER', address, None) + except JsonPatchConflict as e: + ctx = click.get_current_context() + ctx.fail("Invalid ip address. Error: {}".format(e)) tacacs.add_command(delete) diff --git a/config/main.py b/config/main.py index b1badb6c26..b9713e1535 100644 --- a/config/main.py +++ b/config/main.py @@ -6760,73 +6760,82 @@ def is_subintf_shortname(intf): @click.argument('vid', metavar='', required=False, type=click.IntRange(1,4094)) @click.pass_context def add_subinterface(ctx, subinterface_name, vid): + config_db = ValidatedConfigDBConnector(ctx.obj['db']) sub_intf_sep_idx = subinterface_name.find(VLAN_SUB_INTERFACE_SEPARATOR) - if sub_intf_sep_idx == -1: - ctx.fail("{} is invalid vlan subinterface".format(subinterface_name)) - interface_alias = subinterface_name[:sub_intf_sep_idx] - if interface_alias is None: - ctx.fail("{} invalid subinterface".format(interface_alias)) - - if interface_alias.startswith("Po") is True: - intf_table_name = CFG_PORTCHANNEL_PREFIX - elif interface_alias.startswith("Eth") is True: - intf_table_name = 'PORT' - - config_db = ctx.obj['db'] - port_dict = config_db.get_table(intf_table_name) - parent_intf = get_intf_longname(interface_alias) - if interface_alias is not None: - if not port_dict: - ctx.fail("{} parent interface not found. {} table none".format(interface_alias, intf_table_name)) - if parent_intf not in port_dict.keys(): - ctx.fail("{} parent interface not found".format(subinterface_name)) - - # Validate if parent is portchannel member - portchannel_member_table = config_db.get_table('PORTCHANNEL_MEMBER') - if interface_is_in_portchannel(portchannel_member_table, parent_intf): - ctx.fail("{} is configured as a member of portchannel. Cannot configure subinterface" - .format(parent_intf)) + if ADHOC_VALIDATION: + if sub_intf_sep_idx == -1: + ctx.fail("{} is invalid vlan subinterface".format(subinterface_name)) - # Validate if parent is vlan member - vlan_member_table = config_db.get_table('VLAN_MEMBER') - if interface_is_in_vlan(vlan_member_table, parent_intf): - ctx.fail("{} is configured as a member of vlan. Cannot configure subinterface" - .format(parent_intf)) + if interface_alias is None: + ctx.fail("{} invalid subinterface".format(interface_alias)) - sub_intfs = [k for k,v in config_db.get_table('VLAN_SUB_INTERFACE').items() if type(k) != tuple] - if subinterface_name in sub_intfs: - ctx.fail("{} already exists".format(subinterface_name)) + if interface_alias.startswith("Po") is True: + intf_table_name = CFG_PORTCHANNEL_PREFIX + elif interface_alias.startswith("Eth") is True: + intf_table_name = 'PORT' + else: + ctx.fail("{} is invalid vlan subinterface".format(subinterface_name)) + + port_dict = config_db.get_table(intf_table_name) + parent_intf = get_intf_longname(interface_alias) + if interface_alias is not None: + if not port_dict: + ctx.fail("{} parent interface not found. {} table none".format(interface_alias, intf_table_name)) + if parent_intf not in port_dict.keys(): + ctx.fail("{} parent interface not found".format(subinterface_name)) + + # Validate if parent is portchannel member + portchannel_member_table = config_db.get_table('PORTCHANNEL_MEMBER') + if interface_is_in_portchannel(portchannel_member_table, parent_intf): # TODO: MISSING CONSTRAINT IN YANG MODEL + ctx.fail("{} is configured as a member of portchannel. Cannot configure subinterface" + .format(parent_intf)) + + # Validate if parent is vlan member + vlan_member_table = config_db.get_table('VLAN_MEMBER') + if interface_is_in_vlan(vlan_member_table, parent_intf): # TODO: MISSING CONSTRAINT IN YANG MODEL + ctx.fail("{} is configured as a member of vlan. Cannot configure subinterface" + .format(parent_intf)) + + sub_intfs = [k for k,v in config_db.get_table('VLAN_SUB_INTERFACE').items() if type(k) != tuple] + if subinterface_name in sub_intfs: + ctx.fail("{} already exists".format(subinterface_name)) # TODO: MISSING CONSTRAINT IN YANG MODEL + + if subintf_vlan_check(config_db, get_intf_longname(interface_alias), vid) is True: + ctx.fail("Vlan {} encap already configured on other subinterface on {}".format(vid, interface_alias)) # TODO: MISSING CONSTRAINT IN YANG MODEL + + if vid is None and is_subintf_shortname(subinterface_name): + ctx.fail("{} Encap vlan is mandatory or short name subinterfaces".format(subinterface_name)) # TODO: MISSING CONSTRAINT IN YANG MODEL subintf_dict = {} if vid is not None: subintf_dict.update({"vlan" : vid}) - elif is_subintf_shortname(subinterface_name): - ctx.fail("{} Encap vlan is mandatory for short name subinterfaces".format(subinterface_name)) - - if subintf_vlan_check(config_db, get_intf_longname(interface_alias), vid) is True: - ctx.fail("Vlan {} encap already configured on other subinterface on {}".format(vid, interface_alias)) - subintf_dict.update({"admin_status" : "up"}) - config_db.set_entry('VLAN_SUB_INTERFACE', subinterface_name, subintf_dict) + + try: + config_db.set_entry('VLAN_SUB_INTERFACE', subinterface_name, subintf_dict) + except ValueError as e: + ctx.fail("Invalid vlan subinterface. Error: {}".format(e)) @subinterface.command('del') @click.argument('subinterface_name', metavar='', required=True) @click.pass_context def del_subinterface(ctx, subinterface_name): - sub_intf_sep_idx = subinterface_name.find(VLAN_SUB_INTERFACE_SEPARATOR) - if sub_intf_sep_idx == -1: - ctx.fail("{} is invalid vlan subinterface".format(subinterface_name)) + config_db = ValidatedConfigDBConnector(ctx.obj['db']) - config_db = ctx.obj['db'] - #subinterface_name = subintf_get_shortname(subinterface_name) - if interface_name_is_valid(config_db, subinterface_name) is False: - ctx.fail("{} is invalid ".format(subinterface_name)) + if ADHOC_VALIDATION: + sub_intf_sep_idx = subinterface_name.find(VLAN_SUB_INTERFACE_SEPARATOR) + if sub_intf_sep_idx == -1: + ctx.fail("{} is invalid vlan subinterface".format(subinterface_name)) + + #subinterface_name = subintf_get_shortname(subinterface_name) + if interface_name_is_valid(config_db, subinterface_name) is False: + ctx.fail("{} is invalid ".format(subinterface_name)) - subintf_config_db = config_db.get_table('VLAN_SUB_INTERFACE') - sub_intfs = [k for k,v in subintf_config_db.items() if type(k) != tuple] - if subinterface_name not in sub_intfs: - ctx.fail("{} does not exists".format(subinterface_name)) + subintf_config_db = config_db.get_table('VLAN_SUB_INTERFACE') + sub_intfs = [k for k,v in subintf_config_db.items() if type(k) != tuple] + if subinterface_name not in sub_intfs: + ctx.fail("{} does not exists".format(subinterface_name)) ips = {} ips = [ k[1] for k in config_db.get_table('VLAN_SUB_INTERFACE') if type(k) == tuple and k[0] == subinterface_name ] @@ -6842,7 +6851,10 @@ def del_subinterface(ctx, subinterface_name): for ip in ips: config_db.set_entry('INTERFACE', (subinterface_name, ip), None) - config_db.set_entry('VLAN_SUB_INTERFACE', subinterface_name, None) + try: + config_db.set_entry('VLAN_SUB_INTERFACE', subinterface_name, None) + except JsonPatchConflict as e: + ctx.fail("{} is invalid vlan subinterface. Error: {}".format(subinterface_name, e)) if __name__ == '__main__': config() diff --git a/config/validated_config_db_connector.py b/config/validated_config_db_connector.py index b230354ee3..79ebe53244 100644 --- a/config/validated_config_db_connector.py +++ b/config/validated_config_db_connector.py @@ -1,4 +1,5 @@ import jsonpatch +import copy from jsonpointer import JsonPointer from sonic_py_common import device_info @@ -17,33 +18,83 @@ def __getattr__(self, name): return self.validated_set_entry if name == "delete_table": return self.validated_delete_table + if name == "mod_entry": + return self.validated_mod_entry return self.connector.__getattribute__(name) + def stringify_value(self, value): + if isinstance(value, dict): + value = {str(k):str(v) for k, v in value.items()} + else: + value = str(value) + return value + def make_path_value_jsonpatch_compatible(self, table, key, value): if type(key) == tuple: path = JsonPointer.from_parts([table, '|'.join(key)]).path + elif type(key) == list: + path = JsonPointer.from_parts([table, *key]).path else: path = JsonPointer.from_parts([table, key]).path if value == {"NULL" : "NULL"}: value = {} + else: + value = self.stringify_value(value) return path, value - def create_gcu_patch(self, op, table, key=None, value=None): - if key: - path, value = self.make_path_value_jsonpatch_compatible(table, key, value) - else: - path = "/{}".format(table) - + def create_gcu_patch(self, op, table, key=None, value=None, mod_entry=False): gcu_json_input = [] - gcu_json = {"op": "{}".format(op), - "path": "{}".format(path)} - if op == "add": - gcu_json["value"] = value + """Add patch element to create new table if necessary, as GCU is unable to add to nonexistent table""" + if op == "add" and not self.get_table(table): + gcu_json = {"op": "{}".format(op), + "path": "/{}".format(table), + "value": {}} + gcu_json_input.append(gcu_json) + + """Add patch element to create ConfigDB path if necessary, as GCU is unable to add to a nonexistent path""" + if op == "add" and not self.get_entry(table, key): + path = JsonPointer.from_parts([table, key]).path + gcu_json = {"op": "{}".format(op), + "path": "{}".format(path), + "value": {}} + gcu_json_input.append(gcu_json) + + def add_patch_entry(): + if key: + patch_path, patch_value = self.make_path_value_jsonpatch_compatible(table, key, value) + else: + patch_path = "/{}".format(table) + + gcu_json = {"op": "{}".format(op), + "path": "{}".format(patch_path)} + if op == "add": + gcu_json["value"] = patch_value + + gcu_json_input.append(gcu_json) + + """mod_entry makes path more granular so that preexisting fields in db are not removed""" + if mod_entry: + key_start = key + value_copy = copy.deepcopy(value) + for key_end, cleaned_value in value_copy.items(): + key = [key_start, key_end] + value = cleaned_value + add_patch_entry() + else: + add_patch_entry() - gcu_json_input.append(gcu_json) gcu_patch = jsonpatch.JsonPatch(gcu_json_input) return gcu_patch + def apply_patch(self, gcu_patch, table): + format = ConfigFormat.CONFIGDB.name + config_format = ConfigFormat[format.upper()] + + try: + GenericUpdater().apply_patch(patch=gcu_patch, config_format=config_format, verbose=False, dry_run=False, ignore_non_yang_tables=False, ignore_paths=None) + except EmptyTableError: + self.validated_delete_table(table) + def validated_delete_table(self, table): gcu_patch = self.create_gcu_patch("remove", table) format = ConfigFormat.CONFIGDB.name @@ -54,17 +105,20 @@ def validated_delete_table(self, table): logger = genericUpdaterLogging.get_logger(title="Patch Applier", print_all_to_console=True) logger.log_notice("Unable to remove entry, as doing so will result in invalid config. Error: {}".format(e)) + def validated_mod_entry(self, table, key, value): + if value is not None: + op = "add" + else: + op = "remove" + + gcu_patch = self.create_gcu_patch(op, table, key, value, mod_entry=True) + self.apply_patch(gcu_patch, table) + def validated_set_entry(self, table, key, value): if value is not None: op = "add" else: op = "remove" - - gcu_patch = self.create_gcu_patch(op, table, key, value) - format = ConfigFormat.CONFIGDB.name - config_format = ConfigFormat[format.upper()] - try: - GenericUpdater().apply_patch(patch=gcu_patch, config_format=config_format, verbose=False, dry_run=False, ignore_non_yang_tables=False, ignore_paths=None) - except EmptyTableError: - self.validated_delete_table(table) + gcu_patch = self.create_gcu_patch(op, table, key, value) + self.apply_patch(gcu_patch, table) diff --git a/tests/aaa_test.py b/tests/aaa_test.py index 57690e7af7..e554937e1f 100644 --- a/tests/aaa_test.py +++ b/tests/aaa_test.py @@ -4,8 +4,12 @@ from click.testing import CliRunner from utilities_common.db import Db +from jsonpatch import JsonPatchConflict +from unittest import mock +from mock import patch import config.main as config +import config.validated_config_db_connector as validated_config_db_connector import show.main as show test_path = os.path.dirname(os.path.abspath(__file__)) @@ -280,3 +284,27 @@ def test_config_aaa_tacacs(self, get_cmd_module): assert result.exit_code == 0 assert result.output == show_aaa_disable_accounting_output + @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) + @patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=JsonPatchConflict)) + def test_config_aaa_tacacs_delete_yang_validation(self): + config.ADHOC_VALIDATION = True + runner = CliRunner() + db = Db() + obj = {'db':db.cfgdb} + + result = runner.invoke(config.config.commands["tacacs"].commands["delete"], ["10.10.10.10"], obj=obj) + print(result.exit_code) + assert result.exit_code != 0 + + @patch("validated_config_db_connector.device_info.is_yang_config_validation_enabled", mock.Mock(return_value=True)) + @patch("config.validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry", mock.Mock(side_effect=ValueError)) + @patch("config.main.ConfigDBConnector.get_entry", mock.Mock(return_value={})) + def test_config_aaa_tacacs_add_yang_validation(self): + config.ADHOC_VALIDATION = True + runner = CliRunner() + db = Db() + obj = {'db':db.cfgdb} + + result = runner.invoke(config.config.commands["tacacs"].commands["add"], ["10.10.10.10"], obj=obj) + print(result.exit_code) + assert result.exit_code != 0 diff --git a/tests/validated_config_db_connector_test.py b/tests/validated_config_db_connector_test.py index 0e1b608366..48b327e470 100644 --- a/tests/validated_config_db_connector_test.py +++ b/tests/validated_config_db_connector_test.py @@ -18,6 +18,9 @@ SAMPLE_TABLE = 'VLAN' SAMPLE_KEY = 'Vlan1000' SAMPLE_VALUE_EMPTY = None +SAMPLE_VALUE = 'test' +SAMPLE_VALUE_DICT = {'sample_field_key': 'sample_field_value'} +SAMPLE_PATCH = [{"op": "add", "path": "/VLAN", "value": "sample value"}] class TestValidatedConfigDBConnector(TestCase): @@ -30,17 +33,50 @@ def test_validated_set_entry_empty_table(self): mock_generic_updater = mock.Mock() mock_generic_updater.apply_patch = mock.Mock(side_effect=EmptyTableError) with mock.patch('validated_config_db_connector.GenericUpdater', return_value=mock_generic_updater): - remove_entry_success = validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry(mock.Mock(), SAMPLE_TABLE, SAMPLE_KEY, SAMPLE_VALUE_EMPTY) - assert not remove_entry_success + try: + remove_entry_success = validated_config_db_connector.ValidatedConfigDBConnector.validated_set_entry(mock.Mock(), SAMPLE_TABLE, SAMPLE_KEY, SAMPLE_VALUE_EMPTY) + except Exception as ex: + assert False, "Exception {} thrown unexpectedly".format(ex) + + def test_validated_mod_entry(self): + mock_generic_updater = mock.Mock() + with mock.patch('validated_config_db_connector.GenericUpdater', return_value=mock_generic_updater): + try: + successful_application = validated_config_db_connector.ValidatedConfigDBConnector.validated_mod_entry(mock.Mock(), SAMPLE_TABLE, SAMPLE_KEY, SAMPLE_VALUE_DICT) + except Exception as ex: + assert False, "Exception {} thrown unexpectedly".format(ex) def test_validated_delete_table_invalid_delete(self): mock_generic_updater = mock.Mock() mock_generic_updater.apply_patch = mock.Mock(side_effect=ValueError) with mock.patch('validated_config_db_connector.GenericUpdater', return_value=mock_generic_updater): - delete_table_success = validated_config_db_connector.ValidatedConfigDBConnector.validated_delete_table(mock.Mock(), SAMPLE_TABLE) - assert not delete_table_success + try: + delete_table_success = validated_config_db_connector.ValidatedConfigDBConnector.validated_delete_table(mock.Mock(), SAMPLE_TABLE) + except Exception as ex: + assert False, "Exception {} thrown unexpectedly".format(ex) + + def test_create_gcu_patch_set_entry(self): + mock_validated_config_db_connector = ValidatedConfigDBConnector(ConfigDBConnector()) + mock_validated_config_db_connector.get_entry = mock.Mock(return_value=False) + mock_validated_config_db_connector.get_table = mock.Mock(return_value=False) + expected_gcu_patch = jsonpatch.JsonPatch([{"op": "add", "path": "/PORTCHANNEL", "value": {}}, {"op": "add", "path": "/PORTCHANNEL/PortChannel01", "value": {}}, {"op": "add", "path": "/PORTCHANNEL/PortChannel01", "value": "test"}]) + created_gcu_patch = validated_config_db_connector.ValidatedConfigDBConnector.create_gcu_patch(mock_validated_config_db_connector, "add", "PORTCHANNEL", "PortChannel01", SAMPLE_VALUE) + assert expected_gcu_patch == created_gcu_patch - def test_create_gcu_patch(self): - expected_gcu_patch = jsonpatch.JsonPatch([{"op": "add", "path": "/PORTCHANNEL/PortChannel01", "value": "test"}]) - created_gcu_patch = validated_config_db_connector.ValidatedConfigDBConnector.create_gcu_patch(ValidatedConfigDBConnector(ConfigDBConnector()), "add", "PORTCHANNEL", "PortChannel01", "test") + def test_create_gcu_patch_mod_entry(self): + mock_validated_config_db_connector = ValidatedConfigDBConnector(ConfigDBConnector()) + mock_validated_config_db_connector.get_entry = mock.Mock(return_value=True) + mock_validated_config_db_connector.get_table = mock.Mock(return_value=True) + expected_gcu_patch = jsonpatch.JsonPatch([{"op": "add", "path": "/PORTCHANNEL/PortChannel01/test_key", "value": "test_value"}]) + created_gcu_patch = validated_config_db_connector.ValidatedConfigDBConnector.create_gcu_patch(mock_validated_config_db_connector, "add", "PORTCHANNEL", "PortChannel01", {"test_key": "test_value"}, mod_entry=True) assert expected_gcu_patch == created_gcu_patch + + def test_apply_patch(self): + mock_generic_updater = mock.Mock() + mock_generic_updater.apply_patch = mock.Mock(side_effect=EmptyTableError) + with mock.patch('validated_config_db_connector.GenericUpdater', return_value=mock_generic_updater): + with mock.patch('validated_config_db_connector.ValidatedConfigDBConnector.validated_delete_table', return_value=True): + try: + validated_config_db_connector.ValidatedConfigDBConnector.apply_patch(mock.Mock(), SAMPLE_PATCH, SAMPLE_TABLE) + except Exception as ex: + assert False, "Exception {} thrown unexpectedly".format(ex)