YANG Validation for ConfigDB Updates: PORT_STORM_CONTROL, PORT_QOS_MAP, BUFFER_PROFILE, BUFFER_PG, BUFFER_QUEUE, BUFFER_POOL, FEATURE, DEFAULT_LOSSLESS_BUFFER_PARAMETER tables (sonic-net#2498)

isabelmsft · web-flow · commit da44d719c010 · 2022-11-18T23:42:04.000-06:00
diff --git a/config/feature.py b/config/feature.py
@@ -3,6 +3,7 @@
 import click
 from swsscommon import swsscommon
 from utilities_common.cli import AbbreviationGroup, pass_db
+from .validated_config_db_connector import ValidatedConfigDBConnector
 
 SELECT_TIMEOUT = 1000  # ms
 
@@ -24,7 +25,12 @@ def set_feature_state(cfgdb_clients, name, state, block):
         raise Exception("Feature '{}' state is always enabled and can not be modified".format(name))
 
     for ns, cfgdb in cfgdb_clients.items():
-        cfgdb.mod_entry('FEATURE', name, {'state': state})
+        try:
+            config_db = ValidatedConfigDBConnector(cfgdb)
+            config_db.mod_entry('FEATURE', name, {'state': state})
+        except ValueError as e:
+            ctx = click.get_current_context()
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
     if block:
         db = swsscommon.DBConnector('STATE_DB', 0)
@@ -66,7 +72,12 @@ def _update_field(db, name, fld, val):
     if name not in tbl:
         click.echo("Unable to retrieve {} from FEATURE table".format(name))
         sys.exit(1)
-    db.cfgdb.mod_entry('FEATURE', name, { fld: val })
+    try:
+        config_db = ValidatedConfigDBConnector(db.cfgdb)
+        config_db.mod_entry('FEATURE', name, { fld: val })
+    except ValueError as e:
+        ctx = click.get_current_context()
+        ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
 
 #
@@ -137,5 +148,10 @@ def feature_autorestart(db, name, autorestart):
         return
 
     for ns, cfgdb in db.cfgdb_clients.items():
-        cfgdb.mod_entry('FEATURE', name, {'auto_restart': autorestart})
+        try:
+            config_db = ValidatedConfigDBConnector(cfgdb)
+            config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart})
+        except ValueError as e:
+            ctx = click.get_current_context()
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
diff --git a/config/main.py b/config/main.py
@@ -697,17 +697,25 @@ def storm_control_set_entry(port_name, kbps, storm_type, namespace):
         return False
 
     #Validate kbps value
-    config_db = ConfigDBConnector()
+    config_db = ValidatedConfigDBConnector(ConfigDBConnector())
     config_db.connect()
     key = port_name + '|' + storm_type
     entry = config_db.get_entry('PORT_STORM_CONTROL', key)
 
     if len(entry) == 0:
-        config_db.set_entry('PORT_STORM_CONTROL', key, {'kbps':kbps})
+        try:
+            config_db.set_entry('PORT_STORM_CONTROL', key, {'kbps':kbps})
+        except ValueError as e:
+            ctx = click.get_current_context()
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
     else:
         kbps_value = int(entry.get('kbps',0))
         if kbps_value != kbps:
-            config_db.mod_entry('PORT_STORM_CONTROL', key, {'kbps':kbps})
+            try:
+                config_db.mod_entry('PORT_STORM_CONTROL', key, {'kbps':kbps})
+            except ValueError as e:
+                ctx = click.get_current_context()
+                ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
     return True
 
@@ -717,7 +725,7 @@ def storm_control_delete_entry(port_name, storm_type):
     if storm_control_interface_validate(port_name) is False:
         return False
 
-    config_db = ConfigDBConnector()
+    config_db = ValidatedConfigDBConnector(ConfigDBConnector())
     config_db.connect()
     key = port_name + '|' + storm_type
     entry = config_db.get_entry('PORT_STORM_CONTROL', key)
@@ -726,7 +734,11 @@ def storm_control_delete_entry(port_name, storm_type):
         click.echo("%s storm-control not enabled on interface %s" %(storm_type, port_name))
         return False
     else:
-        config_db.set_entry('PORT_STORM_CONTROL', key, None)
+        try:
+            config_db.set_entry('PORT_STORM_CONTROL', key, None)
+        except JsonPatchConflict as e:
+            ctx = click.get_current_context()
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
     return True
 
@@ -4470,7 +4482,7 @@ def _parse_object_id(idsmap):
 
 
 def update_buffer_object(db, interface_name, object_map, override_profile, is_pg, add=True):
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     ctx = click.get_current_context()
 
     # Check whether port is legal
@@ -4500,16 +4512,22 @@ def update_buffer_object(db, interface_name, object_map, override_profile, is_pg
         if is_pg:
             if not 'xoff' in profile_dict.keys() and 'size' in profile_dict.keys():
                 ctx.fail("Profile {} doesn't exist or isn't a lossless profile".format(override_profile))
-        config_db.set_entry(buffer_table, (interface_name, object_map), {"profile": override_profile})
+        try:
+            config_db.set_entry(buffer_table, (interface_name, object_map), {"profile": override_profile})
+        except ValueError as e:
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
     else:
-        config_db.set_entry(buffer_table, (interface_name, object_map), {"profile": "NULL"})
+        try:
+            config_db.set_entry(buffer_table, (interface_name, object_map), {"profile": "NULL"})
+        except ValueError as e:
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
     if is_pg:
         adjust_pfc_enable(ctx, db, interface_name, object_map, True)
 
 
 def remove_buffer_object_on_port(db, interface_name, buffer_object_map, is_pg=True):
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     ctx = click.get_current_context()
 
     # Check whether port is legal
@@ -4530,7 +4548,10 @@ def remove_buffer_object_on_port(db, interface_name, buffer_object_map, is_pg=Tr
                     ctx.fail("Lossy PG {} can't be removed".format(buffer_object_map))
                 else:
                     continue
-            config_db.set_entry(buffer_table, (interface_name, existing_buffer_object), None)
+            try:
+                config_db.set_entry(buffer_table, (interface_name, existing_buffer_object), None)
+            except JsonPatchConflict as e:
+                ctx.fail("Invalid ConfigDB. Error: {}".format(e))
             if is_pg:
                 adjust_pfc_enable(ctx, db, interface_name, buffer_object_map, False)
             removed = True
@@ -4543,7 +4564,7 @@ def remove_buffer_object_on_port(db, interface_name, buffer_object_map, is_pg=Tr
 
 
 def adjust_pfc_enable(ctx, db, interface_name, pg_map, add):
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
 
     # Fetch the original pfc_enable
     qosmap = config_db.get_entry("PORT_QOS_MAP", interface_name)
@@ -4576,7 +4597,10 @@ def adjust_pfc_enable(ctx, db, interface_name, pg_map, add):
         ctx.fail("Try to add empty priorities")
 
     qosmap["pfc_enable"] = pfc_enable[:-1]
-    config_db.set_entry("PORT_QOS_MAP", interface_name, qosmap)
+    try:
+        config_db.set_entry("PORT_QOS_MAP", interface_name, qosmap)
+    except ValueError as e:
+        ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
 
 #
@@ -5811,6 +5835,7 @@ def _is_shared_headroom_pool_enabled(ctx, config_db):
 
 
 def update_profile(ctx, config_db, profile_name, xon, xoff, size, dynamic_th, pool, profile_entry = None):
+    config_db = ValidatedConfigDBConnector(config_db)
     params = {}
     if profile_entry:
         params = profile_entry
@@ -5882,14 +5907,17 @@ def update_profile(ctx, config_db, profile_name, xon, xoff, size, dynamic_th, po
             else:
                 ctx.fail("No dynamic_th defined in DEFAULT_LOSSLESS_BUFFER_PARAMETER")
 
-    config_db.set_entry("BUFFER_PROFILE", (profile_name), params)
+    try:
+        config_db.set_entry("BUFFER_PROFILE", (profile_name), params)
+    except ValueError as e:
+        ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
 @profile.command('remove')
 @click.argument('profile', metavar='<profile>', required=True)
 @clicommon.pass_db
 def remove_profile(db, profile):
     """Delete a buffer profile"""
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     ctx = click.get_current_context()
 
     existing_pgs = config_db.get_table("BUFFER_PG")
@@ -5901,7 +5929,10 @@ def remove_profile(db, profile):
 
     entry = config_db.get_entry("BUFFER_PROFILE", profile)
     if entry:
-        config_db.set_entry("BUFFER_PROFILE", profile, None)
+        try:
+            config_db.set_entry("BUFFER_PROFILE", profile, None)
+        except JsonPatchConflict as e:
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
     else:
         ctx.fail("Profile {} doesn't exist".format(profile))
 
@@ -5917,7 +5948,7 @@ def shared_headroom_pool(ctx):
 @clicommon.pass_db
 def over_subscribe_ratio(db, ratio):
     """Configure over subscribe ratio"""
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     ctx = click.get_current_context()
 
     port_number = len(config_db.get_table('PORT'))
@@ -5937,15 +5968,18 @@ def over_subscribe_ratio(db, ratio):
         else:
             v["over_subscribe_ratio"] = ratio
 
-        config_db.set_entry("DEFAULT_LOSSLESS_BUFFER_PARAMETER", k, v)
+        try:
+            config_db.set_entry("DEFAULT_LOSSLESS_BUFFER_PARAMETER", k, v)
+        except ValueError as e:
+            ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
 
 @shared_headroom_pool.command()
 @click.argument('size', metavar='<size>', type=int, required=True)
 @clicommon.pass_db
 def size(db, size):
     """Configure shared headroom pool size"""
-    config_db = db.cfgdb
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     state_db = db.db
     ctx = click.get_current_context()
 
@@ -5964,7 +5998,10 @@ def size(db, size):
     else:
         ingress_lossless_pool["xoff"] = size
 
-    config_db.set_entry("BUFFER_POOL", "ingress_lossless_pool", ingress_lossless_pool)
+    try:
+        config_db.set_entry("BUFFER_POOL", "ingress_lossless_pool", ingress_lossless_pool)
+    except ValueError as e:
+        ctx.fail("Invalid ConfigDB. Error: {}".format(e))
 
 
 #
diff --git a/config/validated_config_db_connector.py b/config/validated_config_db_connector.py
@@ -53,7 +53,10 @@ def create_gcu_patch(self, op, table, key=None, value=None, mod_entry=False):
 
         """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
+            if type(key) == tuple: 
+                path = JsonPointer.from_parts([table, '|'.join(key)]).path
+            else:
+                path = JsonPointer.from_parts([table, key]).path 
             gcu_json = {"op": "{}".format(op),
                         "path": "{}".format(path),
                         "value": {}}
@@ -106,7 +109,9 @@ def validated_delete_table(self, table):
             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:
+        if isinstance(value, dict) and len(value) == 1 and list(value.values())[0] == "":
+            op = "remove"        
+        elif value is not None:
             op = "add"
         else:
             op = "remove"
@@ -115,7 +120,9 @@ def validated_mod_entry(self, table, key, value):
         self.apply_patch(gcu_patch, table)
 
     def validated_set_entry(self, table, key, value):
-        if value is not None:
+        if isinstance(value, dict) and len(value) == 1 and list(value.values())[0] == "":
+            op = "remove"        
+        elif value is not None:
             op = "add"
         else:
             op = "remove"
diff --git a/tests/buffer_test.py b/tests/buffer_test.py
@@ -2,10 +2,13 @@
 import sys
 import pytest
 import mock
+import jsonpatch
 from importlib import reload
 from click.testing import CliRunner
 from unittest import TestCase
 from swsscommon.swsscommon import ConfigDBConnector
+from mock import patch
+from jsonpatch import JsonPatchConflict
 
 from .mock_tables import dbconnector
 
@@ -14,6 +17,7 @@
 from utilities_common.db import Db
 
 from .buffer_input.buffer_test_vectors import *
+from config.validated_config_db_connector import ValidatedConfigDBConnector
 
 test_path = os.path.dirname(os.path.abspath(__file__))
 modules_path = os.path.dirname(test_path)
@@ -28,6 +32,15 @@ def setup_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "2"
         print("SETUP")
 
+    @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))
+    def test_config_buffer_profile_headroom_yang(self):
+        db = Db()
+        runner = CliRunner()
+        result = runner.invoke(config.config.commands["buffer"].commands["profile"].commands["add"],
+                               ["testprofile", "--dynamic_th", "3", "--xon", "18432", "--xoff", "32768"], obj=db)
+        assert "Invalid ConfigDB. Error" in result.output
+    
     def test_config_buffer_profile_headroom(self):
         runner = CliRunner()
         db = Db()
@@ -118,6 +131,16 @@ def test_config_buffer_profile_multiple_or_none_default_buffer_param_in_database
         assert result.exit_code != 0
         assert "Multiple entries are found in DEFAULT_LOSSLESS_BUFFER_PARAMETER while no dynamic_th specified" in result.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=ValueError))
+    def test_config_shp_size_negative_yang(self):
+        runner = CliRunner()
+        result = runner.invoke(config.config.commands["buffer"].commands["shared-headroom-pool"].commands["size"],
+                               ["200000"])
+        print(result.exit_code)
+        print(result.output)
+        assert "Invalid ConfigDB. Error" in result.output
+
     def test_config_shp_size_negative(self):
         runner = CliRunner()
         result = runner.invoke(config.config.commands["buffer"].commands["shared-headroom-pool"].commands["size"],
@@ -286,7 +309,34 @@ def setup_class(cls):
         os.environ["UTILITIES_UNIT_TESTING"] = "0"
         from .mock_tables import dbconnector
         dbconnector.dedicated_dbs = {}
+    
+    @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))
+    def test_config_int_buffer_pg_lossless_add_yang(self, get_cmd_module):
+        (config, show) = get_cmd_module
+        db = Db()
+        runner = CliRunner()
+        with mock.patch('utilities_common.cli.run_command') as mock_run_command:
+            result = runner.invoke(config.config.commands["interface"].commands["buffer"].commands["priority-group"].
+                                   commands["lossless"].commands["add"],
+                                   ["Ethernet0", "5"], obj=db)
+        assert "Invalid ConfigDB. Error" in result.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_int_buffer_pg_lossless_remove(self, get_cmd_module):
+        (config, show) = get_cmd_module
+        runner = CliRunner()
+        db = Db()
 
+        # Remove non-exist entry
+        with mock.patch('utilities_common.cli.run_command') as mock_run_command:
+            result = runner.invoke(config.config.commands["interface"].commands["buffer"].commands["priority-group"].
+                                   commands["lossless"].commands["remove"],
+                                   ["Ethernet0", "5"], obj=db)
+            print(result.exit_code, result.output)
+            assert "Invalidi ConfigDB. Error" in result.output
+    
     def test_config_int_buffer_pg_lossless_add(self, get_cmd_module):
         (config, show) = get_cmd_module
         runner = CliRunner()
diff --git a/tests/feature_test.py b/tests/feature_test.py
diff --git a/tests/storm_control_test.py b/tests/storm_control_test.py
