From bfb9721599e22801c92a1889b167830da9fcba56 Mon Sep 17 00:00:00 2001 From: isabelmsft <67024108+isabelmsft@users.noreply.github.com> Date: Thu, 19 Jan 2023 14:01:17 -0600 Subject: [PATCH 1/3] resolve merge --- generic_config_updater/generic_updater.py | 4 ++++ generic_config_updater/gu_common.py | 22 +++++++++++++++++++ .../generic_config_updater/gu_common_test.py | 12 ++++++++++ 3 files changed, 38 insertions(+) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 56297039aa..c816d3f2b6 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -47,6 +47,10 @@ def apply(self, patch): # Generate target config self.logger.log_notice("Simulating the target full config after applying the patch.") target_config = self.patch_wrapper.simulate_patch(patch, old_config) + + # Validate all JsonPatch operations on specified fields + self.logger.log_notice("Validating all JsonPatch operations are permitted on the specified fields") + self.config_wrapper.validate_field_operation(old_config, 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, " \ diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 87fb4adfd7..743253ccaf 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -16,6 +16,12 @@ class GenericConfigUpdaterError(Exception): pass +class IllegalPatchOperationError(ValueError): + pass + +class EmptyTableError(ValueError): + pass + class JsonChange: """ A class that describes a partial change to a JSON object. @@ -135,6 +141,22 @@ def validate_config_db_config(self, config_db_as_json): return True, None + def validate_field_operation(self, old_config, target_config): + """ + Some fields in ConfigDB are restricted and may not allow third-party addition, replacement, or removal. + Because YANG only validates state and not transitions, this method helps to JsonPatch operations/transitions for the specified fields. + """ + patch = jsonpatch.JsonPatch.from_diff(old_config, target_config) + + # illegal_operations_to_fields_map['remove'] yields a list of fields for which `remove` is an illegal operation + illegal_operations_to_fields_map = {'add':[], + 'replace': [], + 'remove': ['/PFC_WD/GLOBAL/POLL_INTERVAL', '/PFC_WD/GLOBAL']} + for operation, field_list in illegal_operations_to_fields_map.items(): + for field in field_list: + if any(op['op'] == operation and field == op['path'] for op in patch): + raise IllegalPatchOperationError("Given patch operation is invalid. Operation: {} is illegal on field: {}".format(operation, field)) + def validate_lanes(self, config_db): if "PORT" not in config_db: return True, None diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index a767e641a5..dc18323661 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -69,6 +69,18 @@ def setUp(self): self.config_wrapper_mock = gu_common.ConfigWrapper() self.config_wrapper_mock.get_config_db_as_json=MagicMock(return_value=Files.CONFIG_DB_AS_JSON) + def test_validate_field_operation_legal(self): + old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} + target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "40"}}} + config_wrapper = gu_common.ConfigWrapper() + config_wrapper.validate_field_operation(old_config, target_config) + + def test_validate_field_operation_illegal(self): + old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": 60}}} + target_config = {"PFC_WD": {"GLOBAL": {}}} + config_wrapper = gu_common.ConfigWrapper() + self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config) + def test_ctor__default_values_set(self): config_wrapper = gu_common.ConfigWrapper() From 37eb3163d884f182de171d1e60e85b8778cbf593 Mon Sep 17 00:00:00 2001 From: isabelmsft <67024108+isabelmsft@users.noreply.github.com> Date: Wed, 8 Mar 2023 00:19:03 -0800 Subject: [PATCH 2/3] fix merge conflict --- generic_config_updater/change_applier.py | 2 +- .../field_operation_validators.py | 26 +++++++++ .../gcu_field_operation_validators.conf.json | 20 +++++++ ....json => gcu_services_validator.conf.json} | 0 generic_config_updater/gu_common.py | 36 ++++++++++++ setup.py | 2 +- .../generic_config_updater/gu_common_test.py | 56 +++++++++++++++++-- 7 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 generic_config_updater/field_operation_validators.py create mode 100644 generic_config_updater/gcu_field_operation_validators.conf.json rename generic_config_updater/{generic_config_updater.conf.json => gcu_services_validator.conf.json} (100%) diff --git a/generic_config_updater/change_applier.py b/generic_config_updater/change_applier.py index f5a365d59f..d0818172f8 100644 --- a/generic_config_updater/change_applier.py +++ b/generic_config_updater/change_applier.py @@ -9,7 +9,7 @@ from .gu_common import genericUpdaterLogging SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) -UPDATER_CONF_FILE = f"{SCRIPT_DIR}/generic_config_updater.conf.json" +UPDATER_CONF_FILE = f"{SCRIPT_DIR}/gcu_services_validator.conf.json" logger = genericUpdaterLogging.get_logger(title="Change Applier") print_to_console = False diff --git a/generic_config_updater/field_operation_validators.py b/generic_config_updater/field_operation_validators.py new file mode 100644 index 0000000000..befd4b8749 --- /dev/null +++ b/generic_config_updater/field_operation_validators.py @@ -0,0 +1,26 @@ +from sonic_py_common import device_info +import re + +def rdma_config_update_validator(): + version_info = device_info.get_sonic_version_info() + build_version = version_info.get('build_version') + asic_type = version_info.get('asic_type') + + if (asic_type != 'mellanox' and asic_type != 'broadcom' and asic_type != 'cisco-8000'): + return False + + 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 + break + + if branch_version is None: + return False + + if asic_type == 'cisco-8000': + return branch_version >= "20201200" + else: + return branch_version >= "20181100" diff --git a/generic_config_updater/gcu_field_operation_validators.conf.json b/generic_config_updater/gcu_field_operation_validators.conf.json new file mode 100644 index 0000000000..f12a14d8eb --- /dev/null +++ b/generic_config_updater/gcu_field_operation_validators.conf.json @@ -0,0 +1,20 @@ +{ + "README": [ + "field_operation_validators provides, module & method name as ", + " .", + "NOTE: module name could have '.'", + " ", + "The last element separated by '.' is considered as ", + "method name", + "", + "e.g. 'show.acl.test_acl'", + "", + "field_operation_validators for a given table defines a list of validators that all must pass for modification to the specified field and table to be allowed", + "" + ], + "tables": { + "PFC_WD": { + "field_operation_validators": [ "generic_config_updater.field_operation_validators.rdma_config_update_validator" ] + } + } +} diff --git a/generic_config_updater/generic_config_updater.conf.json b/generic_config_updater/gcu_services_validator.conf.json similarity index 100% rename from generic_config_updater/generic_config_updater.conf.json rename to generic_config_updater/gcu_services_validator.conf.json diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 743253ccaf..57112159ca 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -1,5 +1,6 @@ import json import jsonpatch +import importlib from jsonpointer import JsonPointer import sonic_yang import sonic_yang_ext @@ -7,11 +8,14 @@ import yang as ly import copy import re +import os from sonic_py_common import logger from enum import Enum YANG_DIR = "/usr/local/yang-models" SYSLOG_IDENTIFIER = "GenericConfigUpdater" +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) +GCU_FIELD_OP_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" class GenericConfigUpdaterError(Exception): pass @@ -157,6 +161,38 @@ def validate_field_operation(self, old_config, target_config): if any(op['op'] == operation and field == op['path'] for op in patch): raise IllegalPatchOperationError("Given patch operation is invalid. Operation: {} is illegal on field: {}".format(operation, field)) + def _invoke_validating_function(cmd): + # cmd is in the format as . + method_name = cmd.split(".")[-1] + module_name = ".".join(cmd.split(".")[0:-1]) + if module_name != "generic_config_updater.field_operation_validators" or "validator" not in method_name: + raise GenericConfigUpdaterError("Attempting to call invalid method {} in module {}. Module must be generic_config_updater.field_operation_validators, and method must be a defined validator".format(method_name, module_name)) + module = importlib.import_module(module_name, package=None) + method_to_call = getattr(module, method_name) + return method_to_call() + + if os.path.exists(GCU_FIELD_OP_CONF_FILE): + with open(GCU_FIELD_OP_CONF_FILE, "r") as s: + gcu_field_operation_conf = json.load(s) + else: + raise GenericConfigUpdaterError("GCU field operation validators config file not found") + + for element in patch: + path = element["path"] + match = re.search(r'\/([^\/]+)(\/|$)', path) # This matches the table name in the path, eg if path if /PFC_WD/GLOBAL, the match would be PFC_WD + if match is not None: + table = match.group(1) + else: + raise GenericConfigUpdaterError("Invalid jsonpatch path: {}".format(path)) + validating_functions= set() + tables = gcu_field_operation_conf["tables"] + validating_functions.update(tables.get(table, {}).get("field_operation_validators", [])) + + for function in validating_functions: + if not _invoke_validating_function(function): + raise IllegalPatchOperationError("Modification of {} table is illegal- validating function {} returned False".format(table, function)) + + def validate_lanes(self, config_db): if "PORT" not in config_db: return True, None diff --git a/setup.py b/setup.py index 9b349bc185..48527c44de 100644 --- a/setup.py +++ b/setup.py @@ -63,7 +63,7 @@ 'sonic_cli_gen', ], package_data={ - 'generic_config_updater': ['generic_config_updater.conf.json'], + 'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json'], 'show': ['aliases.ini'], 'sonic_installer': ['aliases.ini'], 'tests': ['acl_input/*', diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index dc18323661..a319a25ead 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -3,8 +3,10 @@ import jsonpatch import sonic_yang import unittest -from unittest.mock import MagicMock, Mock, patch +import mock +from unittest.mock import MagicMock, Mock +from mock import patch from .gutest_helpers import create_side_effect_dict, Files import generic_config_updater.gu_common as gu_common @@ -69,17 +71,61 @@ def setUp(self): self.config_wrapper_mock = gu_common.ConfigWrapper() self.config_wrapper_mock.get_config_db_as_json=MagicMock(return_value=Files.CONFIG_DB_AS_JSON) - def test_validate_field_operation_legal(self): + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "mellanox", "build_version": "SONiC.20181131"})) + def test_validate_field_operation_legal__pfcwd(self): old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "40"}}} config_wrapper = gu_common.ConfigWrapper() config_wrapper.validate_field_operation(old_config, target_config) - - def test_validate_field_operation_illegal(self): - old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": 60}}} + + def test_validate_field_operation_illegal__pfcwd(self): + old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} target_config = {"PFC_WD": {"GLOBAL": {}}} config_wrapper = gu_common.ConfigWrapper() self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config) + + @patch("sonic_py_common.device_info.get_sonic_version_info", mock.Mock(return_value={"asic_type": "invalid-asic", "build_version": "SONiC.20181131"})) + def test_validate_field_modification_illegal__pfcwd(self): + old_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "60"}}} + target_config = {"PFC_WD": {"GLOBAL": {"POLL_INTERVAL": "80"}}} + config_wrapper = gu_common.ConfigWrapper() + self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config) + + def test_validate_field_operation_legal__rm_loopback1(self): + old_config = { + "LOOPBACK_INTERFACE": { + "Loopback0": {}, + "Loopback0|10.1.0.32/32": {}, + "Loopback1": {}, + "Loopback1|10.1.0.33/32": {} + } + } + target_config = { + "LOOPBACK_INTERFACE": { + "Loopback0": {}, + "Loopback0|10.1.0.32/32": {} + } + } + config_wrapper = gu_common.ConfigWrapper() + config_wrapper.validate_field_operation(old_config, target_config) + + def test_validate_field_operation_illegal__rm_loopback0(self): + old_config = { + "LOOPBACK_INTERFACE": { + "Loopback0": {}, + "Loopback0|10.1.0.32/32": {}, + "Loopback1": {}, + "Loopback1|10.1.0.33/32": {} + } + } + target_config = { + "LOOPBACK_INTERFACE": { + "Loopback1": {}, + "Loopback1|10.1.0.33/32": {} + } + } + config_wrapper = gu_common.ConfigWrapper() + self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config) def test_ctor__default_values_set(self): config_wrapper = gu_common.ConfigWrapper() From 5250a157f3c0d1a4712a52e2287a1a04cfe7aa18 Mon Sep 17 00:00:00 2001 From: isabelmsft Date: Thu, 6 Apr 2023 07:04:21 +0000 Subject: [PATCH 3/3] re add assertion --- tests/generic_config_updater/gu_common_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 0787e8faca..a319a25ead 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -124,6 +124,8 @@ def test_validate_field_operation_illegal__rm_loopback0(self): "Loopback1|10.1.0.33/32": {} } } + config_wrapper = gu_common.ConfigWrapper() + self.assertRaises(gu_common.IllegalPatchOperationError, config_wrapper.validate_field_operation, old_config, target_config) def test_ctor__default_values_set(self): config_wrapper = gu_common.ConfigWrapper()