From ddde7db73ff4265e87491b41c79a70b768f9e51e Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 2 Aug 2023 16:19:55 +0000 Subject: [PATCH 01/11] add port validator --- .../field_operation_validators.py | 61 ++++++++- .../field_operation_validators.py.1 | 117 ++++++++++++++++++ .../gcu_field_operation_validators.conf.json | 3 + 3 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 generic_config_updater/field_operation_validators.py.1 diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 8f555d0c1a..005db43b2a 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -5,7 +5,7 @@ import subprocess from sonic_py_common import device_info from .gu_common import GenericConfigUpdaterError - +from swsscommon.swsscommon import SonicV2Connector SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" @@ -71,7 +71,7 @@ def rdma_config_update_validator(patch_element): path = patch_element["path"] table = jsonpointer.JsonPointer(path).parts[0] - # Helper function to return relevant cleaned paths, consdiers case where the jsonpatch value is a dict + # Helper function to return relevant cleaned paths, considers case where the jsonpatch value is a dict # For paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112) def _get_fields_in_patch(): cleaned_fields = [] @@ -126,3 +126,60 @@ def _get_fields_in_patch(): return False return True + + +def read_statedb_entry(table, field): + state_db = SonicV2Connector(host="127.0.0.1") + state_db.connect(state_db.STATE_DB) + return state_db.get(state_db.STATE_DB, table, field) + + +def port_config_update_validator(patch_element): + if patch_element["op"] == "remove": + return True + + # for PORT speed and fec configs, need to ensure value is allowed based on StateDB + patch_element_str = json.dumps(patch_element) + path = patch_element["path"] + match = re.search(r"Ethernet\d+", path) + if match: + port = match.group(0) + else: + return False + value = patch_element.get("value") + + if "fec" in patch_element_str: + if path.endswith("fec"): + fec_value = value + elif isinstance(value, dict): + try: + fec_value = value["fec"] + except KeyError: + return False + + supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs") + if supported_fecs_str: + if supported_fecs_str != 'N/A': + supported_fecs_list = supported_fecs_str.split(',') + else: + supported_fecs_list = [] + else: + supported_fecs_list = [ "rs", "fc", "none"] + if fec_value not in supported_fecs_list: + return False + + if "speed" in patch_element_str: + if path.endswith("speed"): + speed_value = value + elif isinstance(value, dict): + try: + speed_value = value["speed"] + except KeyError: + return False + + supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or '' + supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] + if supported_speeds and int(speed_value) not in supported_speeds: + return False + + return True diff --git a/generic_config_updater/field_operation_validators.py.1 b/generic_config_updater/field_operation_validators.py.1 new file mode 100644 index 0000000000..883944c282 --- /dev/null +++ b/generic_config_updater/field_operation_validators.py.1 @@ -0,0 +1,117 @@ +import os +import re +import json +import jsonpointer +import subprocess +from sonic_py_common import device_info +from .gu_common import GenericConfigUpdaterError + + +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" + +def get_asic_name(): + asic = "unknown" + + if os.path.exists(GCU_TABLE_MOD_CONF_FILE): + with open(GCU_TABLE_MOD_CONF_FILE, "r") as s: + gcu_field_operation_conf = json.load(s) + else: + raise GenericConfigUpdaterError("GCU table modification validators config file not found") + + asic_mapping = gcu_field_operation_conf["helper_data"]["rdma_config_update_validator"] + + if device_info.get_sonic_version_info()['asic_type'] == 'cisco-8000': + asic = "cisco-8000" + elif device_info.get_sonic_version_info()['asic_type'] == 'mellanox': + GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" + spc1_hwskus = asic_mapping["mellanox_asics"]["spc1"] + proc = subprocess.Popen(GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) + output, err = proc.communicate() + hwsku = output.rstrip('\n') + if hwsku.lower() in [spc1_hwsku.lower() for spc1_hwsku in spc1_hwskus]: + asic = "spc1" + elif device_info.get_sonic_version_info()['asic_type'] == 'broadcom': + command = ["sudo", "lspci"] + proc = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE) + output, err = proc.communicate() + broadcom_asics = asic_mapping["broadcom_asics"] + for asic_shorthand, asic_descriptions in broadcom_asics.items(): + if asic != "unknown": + break + for asic_description in asic_descriptions: + if asic_description in output: + asic = asic_shorthand + break + + return asic + + +def rdma_config_update_validator(patch_element): + asic = get_asic_name() + if asic == "unknown": + return False + version_info = device_info.get_sonic_version_info() + build_version = version_info.get('build_version') + version_substrings = build_version.split('.') + branch_version = None + + for substring in version_substrings: + if substring.isdigit() and re.match(r'^\d{8}$', substring): + branch_version = substring + + path = patch_element["path"] + table = jsonpointer.JsonPointer(path).parts[0] + + # Helper function to return relevant cleaned paths, consdiers case where the jsonpatch value is a dict + # For paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112) + def _get_fields_in_patch(): + cleaned_fields = [] + + field_elements = jsonpointer.JsonPointer(path).parts[1:] + cleaned_field_elements = [elem for elem in field_elements if not any(char.isdigit() for char in elem)] + cleaned_field = '/'.join(cleaned_field_elements).lower() + + + if 'value' in patch_element.keys() and isinstance(patch_element['value'], dict): + for key in patch_element['value']: + cleaned_fields.append(cleaned_field+ '/' + key) + else: + cleaned_fields.append(cleaned_field) + + return cleaned_fields + + if os.path.exists(GCU_TABLE_MOD_CONF_FILE): + with open(GCU_TABLE_MOD_CONF_FILE, "r") as s: + gcu_field_operation_conf = json.load(s) + else: + raise GenericConfigUpdaterError("GCU table modification validators config file not found") + + tables = gcu_field_operation_conf["tables"] + scenarios = tables[table]["validator_data"]["rdma_config_update_validator"] + + cleaned_fields = _get_fields_in_patch() + for cleaned_field in cleaned_fields: + scenario = None + for key in scenarios.keys(): + if cleaned_field in scenarios[key]["fields"]: + scenario = scenarios[key] + break + + if scenario is None: + return False + + if scenario["platforms"][asic] == "": + return False + + if patch_element['op'] not in scenario["operations"]: + return False + + if branch_version is not None: + if asic in scenario["platforms"]: + if branch_version < scenario["platforms"][asic]: + return False + else: + return False + + return True diff --git a/generic_config_updater/gcu_field_operation_validators.conf.json b/generic_config_updater/gcu_field_operation_validators.conf.json index f598e0d300..ca56b4e62e 100644 --- a/generic_config_updater/gcu_field_operation_validators.conf.json +++ b/generic_config_updater/gcu_field_operation_validators.conf.json @@ -145,6 +145,9 @@ } } } + }, + "PORT": { + "field_operation_validators": [ "generic_config_updater.field_operation_validators.port_config_update_validator" ] } } } From b13e1ab8845bb75700514754a37b3328d1dd8a55 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 2 Aug 2023 16:20:49 +0000 Subject: [PATCH 02/11] rm extra file --- .../field_operation_validators.py.1 | 117 ------------------ 1 file changed, 117 deletions(-) delete mode 100644 generic_config_updater/field_operation_validators.py.1 diff --git a/generic_config_updater/field_operation_validators.py.1 b/generic_config_updater/field_operation_validators.py.1 deleted file mode 100644 index 883944c282..0000000000 --- a/generic_config_updater/field_operation_validators.py.1 +++ /dev/null @@ -1,117 +0,0 @@ -import os -import re -import json -import jsonpointer -import subprocess -from sonic_py_common import device_info -from .gu_common import GenericConfigUpdaterError - - -SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) -GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" - -def get_asic_name(): - asic = "unknown" - - if os.path.exists(GCU_TABLE_MOD_CONF_FILE): - with open(GCU_TABLE_MOD_CONF_FILE, "r") as s: - gcu_field_operation_conf = json.load(s) - else: - raise GenericConfigUpdaterError("GCU table modification validators config file not found") - - asic_mapping = gcu_field_operation_conf["helper_data"]["rdma_config_update_validator"] - - if device_info.get_sonic_version_info()['asic_type'] == 'cisco-8000': - asic = "cisco-8000" - elif device_info.get_sonic_version_info()['asic_type'] == 'mellanox': - GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" - spc1_hwskus = asic_mapping["mellanox_asics"]["spc1"] - proc = subprocess.Popen(GET_HWSKU_CMD, shell=True, universal_newlines=True, stdout=subprocess.PIPE) - output, err = proc.communicate() - hwsku = output.rstrip('\n') - if hwsku.lower() in [spc1_hwsku.lower() for spc1_hwsku in spc1_hwskus]: - asic = "spc1" - elif device_info.get_sonic_version_info()['asic_type'] == 'broadcom': - command = ["sudo", "lspci"] - proc = subprocess.Popen(command, universal_newlines=True, stdout=subprocess.PIPE) - output, err = proc.communicate() - broadcom_asics = asic_mapping["broadcom_asics"] - for asic_shorthand, asic_descriptions in broadcom_asics.items(): - if asic != "unknown": - break - for asic_description in asic_descriptions: - if asic_description in output: - asic = asic_shorthand - break - - return asic - - -def rdma_config_update_validator(patch_element): - asic = get_asic_name() - if asic == "unknown": - return False - version_info = device_info.get_sonic_version_info() - build_version = version_info.get('build_version') - version_substrings = build_version.split('.') - branch_version = None - - for substring in version_substrings: - if substring.isdigit() and re.match(r'^\d{8}$', substring): - branch_version = substring - - path = patch_element["path"] - table = jsonpointer.JsonPointer(path).parts[0] - - # Helper function to return relevant cleaned paths, consdiers case where the jsonpatch value is a dict - # For paths like /PFC_WD/Ethernet112/action, remove Ethernet112 from the path so that we can clearly determine the relevant field (i.e. action, not Ethernet112) - def _get_fields_in_patch(): - cleaned_fields = [] - - field_elements = jsonpointer.JsonPointer(path).parts[1:] - cleaned_field_elements = [elem for elem in field_elements if not any(char.isdigit() for char in elem)] - cleaned_field = '/'.join(cleaned_field_elements).lower() - - - if 'value' in patch_element.keys() and isinstance(patch_element['value'], dict): - for key in patch_element['value']: - cleaned_fields.append(cleaned_field+ '/' + key) - else: - cleaned_fields.append(cleaned_field) - - return cleaned_fields - - if os.path.exists(GCU_TABLE_MOD_CONF_FILE): - with open(GCU_TABLE_MOD_CONF_FILE, "r") as s: - gcu_field_operation_conf = json.load(s) - else: - raise GenericConfigUpdaterError("GCU table modification validators config file not found") - - tables = gcu_field_operation_conf["tables"] - scenarios = tables[table]["validator_data"]["rdma_config_update_validator"] - - cleaned_fields = _get_fields_in_patch() - for cleaned_field in cleaned_fields: - scenario = None - for key in scenarios.keys(): - if cleaned_field in scenarios[key]["fields"]: - scenario = scenarios[key] - break - - if scenario is None: - return False - - if scenario["platforms"][asic] == "": - return False - - if patch_element['op'] not in scenario["operations"]: - return False - - if branch_version is not None: - if asic in scenario["platforms"]: - if branch_version < scenario["platforms"][asic]: - return False - else: - return False - - return True From 0c58cb85050e65fa9f9f11aba62a1e7e1016bb7e Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sat, 5 Aug 2023 07:29:38 +0000 Subject: [PATCH 03/11] add UT --- .../field_operation_validator_test.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/generic_config_updater/field_operation_validator_test.py b/tests/generic_config_updater/field_operation_validator_test.py index f381c593af..85bff82a1c 100644 --- a/tests/generic_config_updater/field_operation_validator_test.py +++ b/tests/generic_config_updater/field_operation_validator_test.py @@ -14,6 +14,50 @@ class TestValidateFieldOperation(unittest.TestCase): + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_valid_speed_no_state_db(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_valid_speed_existing_state_db(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_valid_speed_existing_state_db(self): + patch_element = {"path": "/PORT/Ethernet3/speed", "op": "add", "value": "234"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_invalid_speed_existing_state_db(self): + patch_element = {"path": "/PORT/Ethernet3/speed", "op": "add", "value": "235"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + + def test_port_config_update_validator_remove(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "remove"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) + def test_port_config_update_validator_invalid_fec_existing_state_db(self): + patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "asf"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) + def test_port_config_update_validator_valid_fec_existing_state_db(self): + patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rs"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_valid_fec_no_state_db(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fec": "rs"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_invalid_fec_no_state_db(self): + patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rsf"} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.get_asic_name", mock.Mock(return_value="unknown")) def test_rdma_config_update_validator_unknown_asic(self): patch_element = {"path": "/PFC_WD/Ethernet4/restoration_time", "op": "replace", "value": "234234"} From 503cedd6ba7e5ffe90c8b9e33ee6d89aaecf5272 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Mon, 7 Aug 2023 23:35:58 +0000 Subject: [PATCH 04/11] update UT --- .../field_operation_validator_test.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/generic_config_updater/field_operation_validator_test.py b/tests/generic_config_updater/field_operation_validator_test.py index 85bff82a1c..2863167350 100644 --- a/tests/generic_config_updater/field_operation_validator_test.py +++ b/tests/generic_config_updater/field_operation_validator_test.py @@ -19,6 +19,11 @@ def test_port_config_update_validator_valid_speed_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_invalid_speed_no_state_db(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"se": "speed"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) def test_port_config_update_validator_valid_speed_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} @@ -53,6 +58,11 @@ def test_port_config_update_validator_valid_fec_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fec": "rs"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_fec_no_state_db_keyerror(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fc": "fec"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) def test_port_config_update_validator_invalid_fec_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rsf"} From 690d871608d0abd57327a5ec15dcd053c83447cb Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Tue, 8 Aug 2023 06:30:36 +0000 Subject: [PATCH 05/11] update validator, UT --- .../field_operation_validators.py | 91 +++++++++++-------- .../field_operation_validator_test.py | 40 ++++++-- 2 files changed, 81 insertions(+), 50 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 005db43b2a..12bc343056 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -135,51 +135,62 @@ def read_statedb_entry(table, field): def port_config_update_validator(patch_element): + + def _validate_key(key, port, value): + if key == "fec": + supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs") + if supported_fecs_str: + if supported_fecs_str != 'N/A': + supported_fecs_list = [element.strip() for element in supported_fecs_str.split(',')] + else: + supported_fecs_list = [] + else: + supported_fecs_list = [ 'rs', 'fc', 'none'] + if value.strip() not in supported_fecs_list: + return False + return True + if key == "speed": + supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or '' + supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] + if supported_speeds and int(value) not in supported_speeds: + return False + return True + + def _parse_port_from_path(path): + match = re.search(r"Ethernet\d+", path) + if match: + port = match.group(0) + return port + return None + if patch_element["op"] == "remove": return True # for PORT speed and fec configs, need to ensure value is allowed based on StateDB patch_element_str = json.dumps(patch_element) path = patch_element["path"] - match = re.search(r"Ethernet\d+", path) - if match: - port = match.group(0) - else: - return False value = patch_element.get("value") - - if "fec" in patch_element_str: - if path.endswith("fec"): - fec_value = value - elif isinstance(value, dict): - try: - fec_value = value["fec"] - except KeyError: - return False - - supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs") - if supported_fecs_str: - if supported_fecs_str != 'N/A': - supported_fecs_list = supported_fecs_str.split(',') - else: - supported_fecs_list = [] - else: - supported_fecs_list = [ "rs", "fc", "none"] - if fec_value not in supported_fecs_list: - return False - - if "speed" in patch_element_str: - if path.endswith("speed"): - speed_value = value - elif isinstance(value, dict): - try: - speed_value = value["speed"] - except KeyError: - return False - - supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or '' - supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] - if supported_speeds and int(speed_value) not in supported_speeds: - return False - + keys = ['fec', 'speed'] + for key in keys: + if key in patch_element_str: + if path.endswith(key): + port = _parse_port_from_path(path) + if not _validate_key(key, port, value): + return False + elif isinstance(value, dict): + if key in value.keys(): + port = _parse_port_from_path(path) + value = value[key] + if not _validate_key(key, port, value): + return False + else: + for port_name, port_info in value.items(): + if isinstance(port_info, dict): + port = port_name + if key in port_info.keys(): + value = port_info[key] + if not _validate_key(key, port, value): + return False + else: + continue return True diff --git a/tests/generic_config_updater/field_operation_validator_test.py b/tests/generic_config_updater/field_operation_validator_test.py index 2863167350..d25052df94 100644 --- a/tests/generic_config_updater/field_operation_validator_test.py +++ b/tests/generic_config_updater/field_operation_validator_test.py @@ -19,11 +19,6 @@ def test_port_config_update_validator_valid_speed_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True - @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) - def test_port_config_update_validator_invalid_speed_no_state_db(self): - patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"se": "speed"}} - assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False - @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) def test_port_config_update_validator_valid_speed_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} @@ -39,6 +34,21 @@ def test_port_config_update_validator_invalid_speed_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3/speed", "op": "add", "value": "235"} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_invalid_speed_existing_state_db_nested(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "235"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_valid_speed_existing_state_db_nested(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "234"}, "Ethernet4": {"alias": "Eth4", "speed": "234"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) + def test_port_config_update_validator_invalid_speed_existing_state_db_nested_2(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "speed": "234"}, "Ethernet4": {"alias": "Eth4", "speed": "236"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + def test_port_config_update_validator_remove(self): patch_element = {"path": "/PORT/Ethernet3", "op": "remove"} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True @@ -48,6 +58,21 @@ def test_port_config_update_validator_invalid_fec_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "asf"} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) + def test_port_config_update_validator_invalid_fec_existing_state_db_nested(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "none"}, "Ethernet4": {"alias": "Eth4", "fec": "fs"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) + def test_port_config_update_validator_valid_fec_existing_state_db_nested(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "fc"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) + def test_port_config_update_validator_valid_fec_existing_state_db_nested_2(self): + patch_element = {"path": "/PORT", "op": "add", "value": {"Ethernet3": {"alias": "Eth0", "fec": "rs"}, "Ethernet4": {"alias": "Eth4", "fec": "fc"}}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="rs, fc")) def test_port_config_update_validator_valid_fec_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rs"} @@ -58,11 +83,6 @@ def test_port_config_update_validator_valid_fec_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fec": "rs"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True - @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) - def test_port_config_update_validator_fec_no_state_db_keyerror(self): - patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"fc": "fec"}} - assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False - @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) def test_port_config_update_validator_invalid_fec_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3/fec", "op": "add", "value": "rsf"} From 0ee717289706461d524269e4973f4d645e5686d9 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sun, 13 Aug 2023 20:05:19 +0000 Subject: [PATCH 06/11] address PR comments --- .../field_operation_validators.py | 38 ++++++++++--------- .../gcu_field_operation_validators.conf.json | 2 +- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 12bc343056..2bd6f5f02e 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -129,15 +129,15 @@ def _get_fields_in_patch(): def read_statedb_entry(table, field): - state_db = SonicV2Connector(host="127.0.0.1") + state_db = SonicV2Connector(use_unix_socket_path=False) state_db.connect(state_db.STATE_DB) return state_db.get(state_db.STATE_DB, table, field) def port_config_update_validator(patch_element): - def _validate_key(key, port, value): - if key == "fec": + def _validate_field(field, port, value): + if field == "fec": supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs") if supported_fecs_str: if supported_fecs_str != 'N/A': @@ -149,12 +149,16 @@ def _validate_key(key, port, value): if value.strip() not in supported_fecs_list: return False return True - if key == "speed": + if field == "speed": supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or '' - supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] - if supported_speeds and int(value) not in supported_speeds: + try: + supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] + if supported_speeds and int(value) not in supported_speeds: + return False + except ValueError: return False return True + return False def _parse_port_from_path(path): match = re.search(r"Ethernet\d+", path) @@ -170,26 +174,26 @@ def _parse_port_from_path(path): patch_element_str = json.dumps(patch_element) path = patch_element["path"] value = patch_element.get("value") - keys = ['fec', 'speed'] - for key in keys: - if key in patch_element_str: - if path.endswith(key): + fields = ['fec', 'speed'] + for field in fields: + if field in patch_element_str: + if path.endswith(field): port = _parse_port_from_path(path) - if not _validate_key(key, port, value): + if not _validate_field(field, port, value): return False elif isinstance(value, dict): - if key in value.keys(): + if field in value.keys(): port = _parse_port_from_path(path) - value = value[key] - if not _validate_key(key, port, value): + value = value[field] + if not _validate_field(field, port, value): return False else: for port_name, port_info in value.items(): if isinstance(port_info, dict): port = port_name - if key in port_info.keys(): - value = port_info[key] - if not _validate_key(key, port, value): + if field in port_info.keys(): + value = port_info[field] + if not _validate_field(field, port, value): return False else: continue diff --git a/generic_config_updater/gcu_field_operation_validators.conf.json b/generic_config_updater/gcu_field_operation_validators.conf.json index ca56b4e62e..28f4dc0508 100644 --- a/generic_config_updater/gcu_field_operation_validators.conf.json +++ b/generic_config_updater/gcu_field_operation_validators.conf.json @@ -146,7 +146,7 @@ } } }, - "PORT": { + "PORT": { "field_operation_validators": [ "generic_config_updater.field_operation_validators.port_config_update_validator" ] } } From f1228da2151fe81c4247eca6be8a0a1a15ee8fbd Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sun, 13 Aug 2023 20:06:29 +0000 Subject: [PATCH 07/11] add UT --- .../generic_config_updater/field_operation_validator_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/generic_config_updater/field_operation_validator_test.py b/tests/generic_config_updater/field_operation_validator_test.py index d25052df94..81a8da5eb7 100644 --- a/tests/generic_config_updater/field_operation_validator_test.py +++ b/tests/generic_config_updater/field_operation_validator_test.py @@ -19,6 +19,11 @@ def test_port_config_update_validator_valid_speed_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) + def test_port_config_update_validator_invalid_speed_no_state_db(self): + patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "xyz"}} + assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="123,234")) def test_port_config_update_validator_valid_speed_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} From dcd95af1b63f7dbb390d1d1795c3354881daaaa0 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Sun, 13 Aug 2023 20:17:48 +0000 Subject: [PATCH 08/11] add constant --- generic_config_updater/field_operation_validators.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 2bd6f5f02e..ea03e2c29c 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -10,6 +10,7 @@ SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" +DEFAULT_SUPPORTED_FECS_LIST = [ 'rs', 'fc', 'none'] def get_asic_name(): asic = "unknown" @@ -145,7 +146,7 @@ def _validate_field(field, port, value): else: supported_fecs_list = [] else: - supported_fecs_list = [ 'rs', 'fc', 'none'] + supported_fecs_list = DEFAULT_SUPPORTED_FECS_LIST if value.strip() not in supported_fecs_list: return False return True From 0e6980ead1a1f82614c8832ede6d3b0119c5dab0 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Mon, 14 Aug 2023 04:28:33 +0000 Subject: [PATCH 09/11] fix UT --- .../generic_config_updater/field_operation_validator_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/generic_config_updater/field_operation_validator_test.py b/tests/generic_config_updater/field_operation_validator_test.py index 81a8da5eb7..f69955fc28 100644 --- a/tests/generic_config_updater/field_operation_validator_test.py +++ b/tests/generic_config_updater/field_operation_validator_test.py @@ -19,8 +19,8 @@ def test_port_config_update_validator_valid_speed_no_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "234"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == True - @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="")) - def test_port_config_update_validator_invalid_speed_no_state_db(self): + @patch("generic_config_updater.field_operation_validators.read_statedb_entry", mock.Mock(return_value="40000,30000")) + def test_port_config_update_validator_invalid_speed_existing_state_db(self): patch_element = {"path": "/PORT/Ethernet3", "op": "add", "value": {"speed": "xyz"}} assert generic_config_updater.field_operation_validators.port_config_update_validator(patch_element) == False From 42d0b696e1c84c58903d0374992b96dba3d80523 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 16 Aug 2023 19:21:25 +0000 Subject: [PATCH 10/11] move fec constant to sonic-utilities constants --- generic_config_updater/field_operation_validators.py | 2 +- scripts/portconfig | 4 +++- utilities_common/constants.py | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index ea03e2c29c..71dac16462 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -6,11 +6,11 @@ from sonic_py_common import device_info from .gu_common import GenericConfigUpdaterError from swsscommon.swsscommon import SonicV2Connector +from utilities_common.constants import DEFAULT_SUPPORTED_FECS_LIST SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) GCU_TABLE_MOD_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku" -DEFAULT_SUPPORTED_FECS_LIST = [ 'rs', 'fc', 'none'] def get_asic_name(): asic = "unknown" diff --git a/scripts/portconfig b/scripts/portconfig index becc203a90..acdebcc236 100755 --- a/scripts/portconfig +++ b/scripts/portconfig @@ -30,6 +30,8 @@ import sys import decimal import argparse +from utilities_common.constants import DEFAULT_SUPPORTED_FECS_LIST + # mock the redis for unit test purposes # try: if os.environ["UTILITIES_UNIT_TESTING"] == "1" or os.environ["UTILITIES_UNIT_TESTING"] == "2": @@ -276,7 +278,7 @@ class portconfig(object): else: supported_fecs_list = [] else: - supported_fecs_list = ["rs", "fc", "none"] + supported_fecs_list = DEFAULT_SUPPORTED_FECS_LIST return supported_fecs_list diff --git a/utilities_common/constants.py b/utilities_common/constants.py index 536965d009..f5c157941c 100644 --- a/utilities_common/constants.py +++ b/utilities_common/constants.py @@ -1,6 +1,7 @@ #All the constant used in sonic-utilities DEFAULT_NAMESPACE = '' +DEFAULT_SUPPORTED_FECS_LIST = [ 'rs', 'fc', 'none'] DISPLAY_ALL = 'all' DISPLAY_EXTERNAL = 'frontend' BGP_NEIGH_OBJ = 'BGP_NEIGH' From e2bd8322e5d34a8298d317e89a3a9f595e4cbdb3 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Wed, 16 Aug 2023 20:54:45 +0000 Subject: [PATCH 11/11] use Table class --- .../field_operation_validators.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py index 71dac16462..fc7c14464e 100644 --- a/generic_config_updater/field_operation_validators.py +++ b/generic_config_updater/field_operation_validators.py @@ -5,7 +5,7 @@ import subprocess from sonic_py_common import device_info from .gu_common import GenericConfigUpdaterError -from swsscommon.swsscommon import SonicV2Connector +from swsscommon import swsscommon from utilities_common.constants import DEFAULT_SUPPORTED_FECS_LIST SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) @@ -129,17 +129,17 @@ def _get_fields_in_patch(): return True -def read_statedb_entry(table, field): - state_db = SonicV2Connector(use_unix_socket_path=False) - state_db.connect(state_db.STATE_DB) - return state_db.get(state_db.STATE_DB, table, field) +def read_statedb_entry(table, key, field): + state_db = swsscommon.DBConnector("STATE_DB", 0) + tbl = swsscommon.Table(state_db, table) + return tbl.hget(key, field)[1] def port_config_update_validator(patch_element): def _validate_field(field, port, value): if field == "fec": - supported_fecs_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_fecs") + supported_fecs_str = read_statedb_entry("PORT_TABLE", port, "supported_fecs") if supported_fecs_str: if supported_fecs_str != 'N/A': supported_fecs_list = [element.strip() for element in supported_fecs_str.split(',')] @@ -151,7 +151,7 @@ def _validate_field(field, port, value): return False return True if field == "speed": - supported_speeds_str = read_statedb_entry('{}|{}'.format("PORT_TABLE", port), "supported_speeds") or '' + supported_speeds_str = read_statedb_entry("PORT_TABLE", port, "supported_speeds") or '' try: supported_speeds = [int(s) for s in supported_speeds_str.split(',') if s] if supported_speeds and int(value) not in supported_speeds: