YANG Validation for ConfigDB Updates: TACPLUS, TACPLUS_SERVER, AAA, VLAN_SUB_INTERFACE tables + decorated validated_mod_entry (sonic-net#2452)

Author: isabelmsft

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='<ip_address>')
 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)
 
 

config/main.py

@@ -6760,73 +6760,82 @@ def is_subintf_shortname(intf):
 @click.argument('vid', metavar='<vid>', 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='<subinterface_name>', 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()

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)