YANG Validation for ConfigDB Updates: DEVICE_METADATA, SNMP, SNMP_COMMUNITY tables (sonic-net#2481)

Author: isabelmsft

config/main.py

@@ -1944,11 +1944,15 @@ def override_config_db(config_db, config_input):
 @click.argument('new_hostname', metavar='<new_hostname>', required=True)
 def hostname(new_hostname):
     """Change device hostname without impacting the traffic."""
-
-    config_db = ConfigDBConnector()
+    config_db = ValidatedConfigDBConnector(ConfigDBConnector())
     config_db.connect()
-    config_db.mod_entry(swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, 'localhost',
-                        {'hostname': new_hostname})
+    try:
+        config_db.mod_entry(swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, 'localhost',
+                            {'hostname': new_hostname})
+    except ValueError as e:
+        ctx = click.get_current_context()
+        ctx.fail("Failed to write new hostname to ConfigDB. Error: {}".format(e))
+
 
     click.echo('Please note loaded setting will be lost after system reboot. To'
                ' preserve setting, run `config save`.')
@@ -1966,32 +1970,42 @@ def synchronous_mode(sync_mode):
                config reload -y \n
             2. systemctl restart swss
     """
-
-    if sync_mode == 'enable' or sync_mode == 'disable':
-        config_db = ConfigDBConnector()
-        config_db.connect()
+    if ADHOC_VALIDATION:
+        if sync_mode != 'enable' and sync_mode != 'disable':
+            raise click.BadParameter("Error: Invalid argument %s, expect either enable or disable" % sync_mode)
+        
+    config_db = ValidatedConfigDBConnector(ConfigDBConnector())
+    config_db.connect()
+    try:
         config_db.mod_entry('DEVICE_METADATA' , 'localhost', {"synchronous_mode" : sync_mode})
-        click.echo("""Wrote %s synchronous mode into CONFIG_DB, swss restart required to apply the configuration: \n
+    except ValueError as e:
+        ctx = click.get_current_context()
+        ctx.fail("Error: Invalid argument %s, expect either enable or disable" % sync_mode)
+    
+    click.echo("""Wrote %s synchronous mode into CONFIG_DB, swss restart required to apply the configuration: \n
     Option 1. config save -y \n
               config reload -y \n
     Option 2. systemctl restart swss""" % sync_mode)
-    else:
-        raise click.BadParameter("Error: Invalid argument %s, expect either enable or disable" % sync_mode)
 
 #
 # 'yang_config_validation' command ('config yang_config_validation ...')
 # 
 @config.command('yang_config_validation')
 @click.argument('yang_config_validation', metavar='<enable|disable>', required=True)
 def yang_config_validation(yang_config_validation):
-    # Enable or disable YANG validation on updates to ConfigDB
-    if yang_config_validation == 'enable' or yang_config_validation == 'disable':
-        config_db = ConfigDBConnector()
-        config_db.connect()
+    if ADHOC_VALIDATION:
+        if yang_config_validation != 'enable' and yang_config_validation != 'disable':
+            raise click.BadParameter("Error: Invalid argument %s, expect either enable or disable" % yang_config_validation)
+
+    config_db = ValidatedConfigDBConnector(ConfigDBConnector())
+    config_db.connect()
+    try:
         config_db.mod_entry('DEVICE_METADATA', 'localhost', {"yang_config_validation": yang_config_validation})
-        click.echo("""Wrote %s yang config validation into CONFIG_DB""" % yang_config_validation)
-    else:
-        raise click.BadParameter("Error: Invalid argument %s, expect either enable or disable" % yang_config_validation)
+    except ValueError as e:
+        ctx = click.get_current_context()
+        ctx.fail("Error: Invalid argument %s, expect either enable or disable" % yang_config_validation)
+
+    click.echo("""Wrote %s yang config validation into CONFIG_DB""" % yang_config_validation)
 
 #
 # 'portchannel' group ('config portchannel ...')
@@ -3193,16 +3207,24 @@ def snmp_user_secret_check(snmp_secret):
 def add_community(db, community, string_type):
     """ Add snmp community string"""
     string_type = string_type.upper()
-    if not is_valid_community_type(string_type):
-        sys.exit(1)
-    if not snmp_community_secret_check(community):
-        sys.exit(2)
-    snmp_communities = db.cfgdb.get_table("SNMP_COMMUNITY")
-    if community in snmp_communities:
-        click.echo("SNMP community {} is already configured".format(community))
-        sys.exit(3)
-    db.cfgdb.set_entry('SNMP_COMMUNITY', community, {'TYPE': string_type})
-    click.echo("SNMP community {} added to configuration".format(community))
+    if ADHOC_VALIDATION:
+        if not is_valid_community_type(string_type):
+            sys.exit(1)
+        if not snmp_community_secret_check(community):
+            sys.exit(2)
+        snmp_communities = db.cfgdb.get_table("SNMP_COMMUNITY")
+        if community in snmp_communities:
+            click.echo("SNMP community {} is already configured".format(community))
+            sys.exit(3)
+
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
+    try:
+        config_db.set_entry('SNMP_COMMUNITY', community, {'TYPE': string_type})
+        click.echo("SNMP community {} added to configuration".format(community))
+    except ValueError as e:
+        ctx = click.get_current_context()
+        ctx.fail("SNMP community configuration failed. Error: {}".format(e))
+
     try:
         click.echo("Restarting SNMP service...")
         clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)
@@ -3217,20 +3239,27 @@ def add_community(db, community, string_type):
 @clicommon.pass_db
 def del_community(db, community):
     """ Delete snmp community string"""
-    snmp_communities = db.cfgdb.get_table("SNMP_COMMUNITY")
-    if community not in snmp_communities:
-        click.echo("SNMP community {} is not configured".format(community))
-        sys.exit(1)
-    else:
-        db.cfgdb.set_entry('SNMP_COMMUNITY', community, None)
+    if ADHOC_VALIDATION:
+        snmp_communities = db.cfgdb.get_table("SNMP_COMMUNITY")
+        if community not in snmp_communities:
+            click.echo("SNMP community {} is not configured".format(community))
+            sys.exit(1)
+    
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
+    try:
+        config_db.set_entry('SNMP_COMMUNITY', community, None)
         click.echo("SNMP community {} removed from configuration".format(community))
-        try:
-            click.echo("Restarting SNMP service...")
-            clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)
-            clicommon.run_command("systemctl restart snmp.service", display_cmd=False)
-        except SystemExit as e:
-            click.echo("Restart service snmp failed with error {}".format(e))
-            raise click.Abort()
+    except JsonPatchConflict as e:
+        ctx = click.get_current_context()
+        ctx.fail("SNMP community {} is not configured. Error: {}".format(community, e))
+
+    try:
+        click.echo("Restarting SNMP service...")
+        clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)
+        clicommon.run_command("systemctl restart snmp.service", display_cmd=False)
+    except SystemExit as e:
+        click.echo("Restart service snmp failed with error {}".format(e))
+        raise click.Abort()
 
 
 @community.command('replace')
@@ -3286,7 +3315,7 @@ def add_contact(db, contact, contact_email):
             click.echo("Contact already exists.  Use sudo config snmp contact modify instead")
             sys.exit(1)
         else:
-            db.cfgdb.set_entry('SNMP', 'CONTACT', {contact: contact_email})
+            db.cfgdb.set_entry('SNMP', 'CONTACT', {contact: contact_email}) # TODO: ERROR IN YANG MODEL. Contact name is not defined as key
             click.echo("Contact name {} and contact email {} have been added to "
                        "configuration".format(contact, contact_email))
             try:
@@ -3399,19 +3428,24 @@ def location(db):
 @clicommon.pass_db
 def add_location(db, location):
     """ Add snmp location"""
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     if isinstance(location, tuple):
         location = " ".join(location)
     elif isinstance(location, list):
         location = " ".join(location)
-    snmp = db.cfgdb.get_table("SNMP")
+    snmp = config_db.get_table("SNMP")
     try:
         if snmp['LOCATION']:
             click.echo("Location already exists")
             sys.exit(1)
     except KeyError:
         if "LOCATION" not in snmp.keys():
-            db.cfgdb.set_entry('SNMP', 'LOCATION', {'Location': location})
-            click.echo("SNMP Location {} has been added to configuration".format(location))
+            try:
+                config_db.set_entry('SNMP', 'LOCATION', {'Location': location})
+                click.echo("SNMP Location {} has been added to configuration".format(location))
+            except ValueError:
+                ctx = click.get_current_context()
+                ctx.fail("Failed to set SNMP location. Error: {}".format(e))
             try:
                 click.echo("Restarting SNMP service...")
                 clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)
@@ -3426,15 +3460,20 @@ def add_location(db, location):
 @clicommon.pass_db
 def delete_location(db, location):
     """ Delete snmp location"""
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     if isinstance(location, tuple):
         location = " ".join(location)
     elif isinstance(location, list):
         location = " ".join(location)
     snmp = db.cfgdb.get_table("SNMP")
     try:
         if location == snmp['LOCATION']['Location']:
-            db.cfgdb.set_entry('SNMP', 'LOCATION', None)
-            click.echo("SNMP Location {} removed from configuration".format(location))
+            try:
+                config_db.set_entry('SNMP', 'LOCATION', None)
+                click.echo("SNMP Location {} removed from configuration".format(location))
+            except (ValueError, JsonPatchConflict) as e:
+                ctx = click.get_current_context()
+                ctx.fail("Failed to remove SNMP location from configuration. Error: {}".format(e))
             try:
                 click.echo("Restarting SNMP service...")
                 clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)
@@ -3456,19 +3495,24 @@ def delete_location(db, location):
 @clicommon.pass_db
 def modify_location(db, location):
     """ Modify snmp location"""
+    config_db = ValidatedConfigDBConnector(db.cfgdb)
     if isinstance(location, tuple):
         location = " ".join(location)
     elif isinstance(location, list):
         location = " ".join(location)
-    snmp = db.cfgdb.get_table("SNMP")
+    snmp = config_db.get_table("SNMP")
     try:
         snmp_location = snmp['LOCATION']['Location']
         if location in snmp_location:
             click.echo("SNMP location {} already exists".format(location))
             sys.exit(1)
         else:
-            db.cfgdb.mod_entry('SNMP', 'LOCATION', {'Location': location})
-            click.echo("SNMP location {} modified in configuration".format(location))
+            try:
+                config_db.mod_entry('SNMP', 'LOCATION', {'Location': location})
+                click.echo("SNMP location {} modified in configuration".format(location))
+            except ValueError as e:
+                ctx = click.get_current_context()
+                ctx.fail("Failed to modify SNMP location. Error: {}".format(e))
             try:
                 click.echo("Restarting SNMP service...")
                 clicommon.run_command("systemctl reset-failed snmp.service", display_cmd=False)

tests/config_snmp_test.py

@@ -6,6 +6,7 @@
 import show.main as show
 import clear.main as clear
 import config.main as config
+import config.validated_config_db_connector as validated_config_db_connector
 
 import pytest
 
@@ -865,6 +866,16 @@ def test_is_valid_email(self, invalid_email):
         output = config.is_valid_email(invalid_email)
         assert output == False
 
+    @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_snmp_community_add_new_community_with_invalid_type_yang_validation(self):
+        config.ADHOC_VALIDATION = False
+        runner = CliRunner()
+        result = runner.invoke(config.config.commands["snmp"].commands["community"].commands["add"], ["Everest", "RT"])
+        print(result.exit_code)
+        assert result.exit_code != 0
+        assert 'SNMP community configuration failed' in result.output
+
     @classmethod
     def teardown_class(cls):
         print("TEARDOWN")

tests/config_test.py

@@ -1603,7 +1603,7 @@ def setup_class(cls):
         import config.main
         importlib.reload(config.main)
 
-    @mock.patch('config.main.ConfigDBConnector')
+    @mock.patch('config.main.ValidatedConfigDBConnector')
     def test_hostname_add(self, db_conn_patch, get_cmd_module):
         db_conn_patch().mod_entry = mock.Mock()
         (config, show) = get_cmd_module
@@ -1625,6 +1625,19 @@ def test_hostname_add(self, db_conn_patch, get_cmd_module):
         # Check new hostname was part of args
         assert {'hostname': 'new_hostname'} in args
 
+    @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_mod_entry", mock.Mock(side_effect=ValueError))
+    def test_invalid_hostname_add_yang_validation(self):
+        config.ADHOC_VALIDATION = False
+        runner = CliRunner()
+        db = Db()
+        obj = {'db':db.cfgdb}
+
+        result = runner.invoke(config.config.commands["hostname"],
+                               ["invalid_hostname"], obj=obj)
+        assert result.exit_code != 0
+        assert "Failed to write new hostname" in result.output
+
     @classmethod
     def teardown_class(cls):
         print("TEARDOWN")

tests/synchronous_mode_test.py

@@ -1,5 +1,9 @@
 from click.testing import CliRunner
+from utilities_common.db import Db
+from unittest import mock
+from mock import patch
 import config.main as config
+import config.validated_config_db_connector as validated_config_db_connector 
 
 class TestSynchronousMode(object):
     @classmethod
@@ -11,7 +15,7 @@ def __check_result(self, result_msg, mode):
             expected_msg = """Wrote %s synchronous mode into CONFIG_DB, swss restart required to apply the configuration: \n
     Option 1. config save -y \n
               config reload -y \n
-    Option 2. systemctl restart swss""" % mode
+    Option 2. systemctl restart swss"""  % mode
         else:
             expected_msg = "Error: Invalid argument %s, expect either enable or disable" % mode
 
@@ -35,3 +39,17 @@ def test_synchronous_mode(self):
         print(result.output)
         assert result.exit_code != 0
         assert self.__check_result(result.output, "invalid-input")
+
+    @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_mod_entry", mock.Mock(side_effect=ValueError))
+    def test_synchronous_mode_invalid_yang_validation(self):
+        config.ADHOC_VALIDATION = False
+        runner = CliRunner()
+        db = Db()
+        obj = {'db':db.cfgdb}
+
+        result = runner.invoke(config.config.commands["synchronous_mode"], ["invalid-input"], obj=obj)
+        print(result.exit_code)
+        print(result.output)
+        assert result.exit_code != 0
+        assert self.__check_result(result.output, "invalid-input")

tests/tpid_test.py

@@ -101,7 +101,8 @@ def test_tpid_config_lag_mbr(self):
 
     def test_tpid_add_lag_mbr_with_non_default_tpid(self):
         db = Db()
-        obj = {'db':db.cfgdb}
+        obj = {'db': db.cfgdb}
+        config.ADHOC_VALIDATION = True
         runner = CliRunner()
         result = runner.invoke(config.config.commands["portchannel"].commands["member"].commands["add"], ["PortChannel0001","Ethernet20"], obj=obj)
         print(result.exit_code)

tests/yang_config_validation_test.py

@@ -1,5 +1,9 @@
 from click.testing import CliRunner
+from utilities_common.db import Db
+from unittest import mock
+from mock import patch
 import config.main as config
+import config.validated_config_db_connector as validated_config_db_connector
 
 class TestYangConfigValidation(object):
     @classmethod
@@ -15,6 +19,7 @@ def __check_result(self, result_msg, mode):
         return expected_msg in result_msg
 
     def test_yang_config_validation(self):
+        config.ADHOC_VALIDATION = True
         runner = CliRunner()
 
         result = runner.invoke(config.config.commands["yang_config_validation"], ["enable"])
@@ -31,3 +36,17 @@ def test_yang_config_validation(self):
         print(result.output)
         assert result.exit_code != 0
         assert self.__check_result(result.output, "invalid-input")
+
+    @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_mod_entry", mock.Mock(side_effect=ValueError))
+    def test_invalid_yang_config_validation_using_yang(self):
+        config.ADHOC_VALIDATION = False
+        runner = CliRunner()
+        db = Db()
+        obj = {'db':db.cfgdb}
+
+        result = runner.invoke(config.config.commands["yang_config_validation"], ["invalid-input"], obj=obj)
+        print(result.exit_code)
+        print(result.output)
+        assert result.exit_code != 0
+        assert self.__check_result(result.output, "invalid-input")