From db95f248a747e27a4a65e5f26835495818aa033b Mon Sep 17 00:00:00 2001 From: Brad House Date: Mon, 3 Mar 2025 21:43:53 -0500 Subject: [PATCH 1/2] sonic-yang-mgmt: uses clause with leaf-list, choice not honored When a uses clause imports a grouping, it was only processing leaf entries and ignoring leaf-list and choice clauses. That means that for instance in bgp route maps, route_map_in and route_map_out validations would fail. Honoring the uses `refine` clause is now also honored which is depended upon in sonic-utilities. This now precompiles the uses clause and integrates it as if the uses clause was not part of the schema as multiple end users were having to do this additional processing. This fixes that behavior and adds test cases to ensure it doesn't regress in the future. Signed-off-by: Brad House (@bradh352) --- src/sonic-yang-mgmt/sonic_yang_ext.py | 147 ++++++++++++------ .../tests/files/sample_config_db.json | 4 + .../tests/yang_model_tests/tests/bgp.json | 6 + .../yang_model_tests/tests_config/bgp.json | 112 +++++++++++++ 4 files changed, 225 insertions(+), 44 deletions(-) diff --git a/src/sonic-yang-mgmt/sonic_yang_ext.py b/src/sonic-yang-mgmt/sonic_yang_ext.py index 6214e6885bd..42b4767068e 100644 --- a/src/sonic-yang-mgmt/sonic_yang_ext.py +++ b/src/sonic-yang-mgmt/sonic_yang_ext.py @@ -72,6 +72,8 @@ def loadYangModel(self): self._loadJsonYangModel() # create a map from config DB table to yang container self._createDBTableToModuleMap() + # compile uses clause (embed into schema) + self._compileUsesClause() except Exception as e: self.sysLog(msg="Yang Models Load failed:{}".format(str(e)), \ debug=syslog.LOG_ERR, doPrint=True) @@ -130,8 +132,10 @@ def _preProcessYangGrouping(self, moduleName, module): for grouping in groupings: gName = grouping["@name"] - gLeaf = grouping["leaf"] - self.preProcessedYang['grouping'][moduleName][gName] = gLeaf + self.preProcessedYang['grouping'][moduleName][gName] = dict() + self.preProcessedYang['grouping'][moduleName][gName]["leaf"] = grouping.get('leaf') + self.preProcessedYang['grouping'][moduleName][gName]["leaf-list"] = grouping.get('leaf-list') + self.preProcessedYang['grouping'][moduleName][gName]["choice"] = grouping.get('choice') except Exception as e: self.sysLog(msg="_preProcessYangGrouping failed:{}".format(str(e)), \ @@ -162,6 +166,103 @@ def _preProcessYang(self, moduleName, module): raise e return + def _compileUsesClauseRefineApply(self, refine, model): + for item in model: + if item["@name"] == refine["@target-node"]: + # NOTE: We only handle 'description' and 'mandatory' + if refine.get('description') is not None: + item["description"] = refine["description"] + if refine.get('mandatory') is not None: + item["mandatory"] = refine["mandatory"] + return + + def _compileUsesClauseRefine(self, refine, model): + # Nothing to refine + if refine is None: + return + + # Convert to list if not already + if not isinstance(refine, list): + refine = [ refine ] + + # Iterate across each refine + for r in refine: + self._compileUsesClauseRefineApply(r, model) + + + def _compileUsesClauseList(self, model, group, name, refine): + # If group doesn't have this entry, nothing to do. + if not group.get(name): + return + + # model doesn't have this entry type, create it as a list + if not model.get(name): + model[name] = [] + + # model has this entry type, but its not a list, convert + if not isinstance(model.get(name), list): + model[name] = [ model[name] ] + + if isinstance(group.get(name), list): + model[name].extend(group.get(name)) + else: + model[name].append(group.get(name)) + + self._compileUsesClauseRefine(refine, model[name]) + + # Recursively process the yang schema looking for the "uses" clause under + # "container", "list", and "choice"/"case" nodes. Merge in the "uses" + # dictionaries for leaf and leaf-list so callers don't need to try to do + # their own "uses" processing. Remove the "uses" member when processed so + # anyone expecting it won't try to re-process. It will just look like a + # yang model that doesn't use "uses" so shouldn't cause compatibility issues. + def _compileUsesClauseModel(self, module, model): + if isinstance(model, list): + for item in model: + self._compileUsesClauseModel(module, item) + return + + for model_name in [ "container", "list", "choice", "case" ]: + node = model.get(model_name) + if node: + self._compileUsesClauseModel(module, node) + + uses_s = model.get("uses") + if not uses_s: + return + + # Always make as a list + if isinstance(uses_s, dict): + uses_s = [uses_s] + + # uses Example: "@name": "bgpcmn:sonic-bgp-cmn" + for uses in uses_s: + # Assume ':' means reference to another module + if ':' in uses['@name']: + prefix = uses['@name'].split(':')[0].strip() + uses_module_name = self._findYangModuleFromPrefix(prefix, module) + else: + uses_module_name = module['@name'] + grouping = uses['@name'].split(':')[-1].strip() + groupdata = self.preProcessedYang['grouping'][uses_module_name][grouping] + + # Merge leaf from uses + refine = uses.get("refine") + self._compileUsesClauseList(model, groupdata, 'leaf', refine) + self._compileUsesClauseList(model, groupdata, 'leaf-list', refine) + self._compileUsesClauseList(model, groupdata, 'choice', refine) + + # Delete the uses node so callers don't use it. + del model["uses"] + + def _compileUsesClause(self): + try: + for module in self.yJson: + self._compileUsesClauseModel(module["module"], module["module"]) + except Exception as e: + traceback.print_exc() + raise e + """ Create a map from config DB tables to container in yang model This module name and topLevelContainer are fetched considering YANG models are @@ -328,44 +429,6 @@ def _findYangModuleFromPrefix(self, prefix, module): raise e return None - def _fillLeafDictUses(self, uses_s, table, leafDict): - ''' - Find the leaf(s) in a grouping which maps to given uses statement, - then fill leafDict with leaf(s) information. - - Parameters: - uses_s (str): uses statement in yang module. - table (str): config DB table, this table is being translated. - leafDict (dict): dict with leaf(s) information for List\Container - corresponding to config DB table. - - Returns: - (void) - ''' - try: - # make a list - if isinstance(uses_s, dict): - uses_s = [uses_s] - # find yang module for current table - table_module = self.confDbYangMap[table]['yangModule'] - # uses Example: "@name": "bgpcmn:sonic-bgp-cmn" - for uses in uses_s: - # Assume ':' means reference to another module - if ':' in uses['@name']: - prefix = uses['@name'].split(':')[0].strip() - uses_module_name = self._findYangModuleFromPrefix(prefix, table_module) - else: - uses_module_name = table_module['@name'] - grouping = uses['@name'].split(':')[-1].strip() - leafs = self.preProcessedYang['grouping'][uses_module_name][grouping] - self._fillLeafDict(leafs, leafDict) - except Exception as e: - self.sysLog(msg="_fillLeafDictUses failed:{}".format(str(e)), \ - debug=syslog.LOG_ERR, doPrint=True) - raise e - - return - def _createLeafDict(self, model, table): ''' create a dict to map each key under primary key with a leaf in yang model. @@ -402,10 +465,6 @@ def _createLeafDict(self, model, table): # leaf-lists self._fillLeafDict(model.get('leaf-list'), leafDict, True) - # uses should map to grouping, - if model.get('uses') is not None: - self._fillLeafDictUses(model.get('uses'), table, leafDict) - return leafDict """ diff --git a/src/sonic-yang-models/tests/files/sample_config_db.json b/src/sonic-yang-models/tests/files/sample_config_db.json index cd34eb9defd..753091c04ae 100644 --- a/src/sonic-yang-models/tests/files/sample_config_db.json +++ b/src/sonic-yang-models/tests/files/sample_config_db.json @@ -1912,6 +1912,8 @@ }, "BGP_NEIGHBOR_AF": { "default|192.168.1.1|ipv4_unicast": { + "route_map_in": [ "map1" ], + "route_map_out": [ "map1" ] } }, "BGP_PEER_GROUP": { @@ -1920,6 +1922,8 @@ }, "BGP_PEER_GROUP_AF": { "default|PG1|ipv4_unicast": { + "route_map_in": [ "map1" ], + "route_map_out": [ "map1" ] } }, "BGP_GLOBALS_LISTEN_PREFIX": { diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests/bgp.json b/src/sonic-yang-models/tests/yang_model_tests/tests/bgp.json index 476c9c2d47c..52c696f03a5 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests/bgp.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests/bgp.json @@ -90,6 +90,9 @@ "eStrKey" : "InvalidValue", "eStr": ["send_community"] }, + "BGP_NEIGHBOR_AF_ROUTE_MAP_VALID": { + "desc": "Reference valid route map" + }, "BGP_NEIGHBOR_AF_NEG_ROUTE_MAP_NOT_EXIST": { "desc": "Reference to non-existent route-map.", "eStrKey" : "LeafRef" @@ -134,6 +137,9 @@ "eStrKey" : "InvalidValue", "eStr": ["send_community"] }, + "BGP_PEERGROUP_AF_ROUTE_MAP_VALID": { + "desc": "Validate route-map reference in peergroup AF." + }, "BGP_PEERGROUP_AF_ROUTE_MAP_NOT_EXIST": { "desc": "Missing or non-existent route-map reference in peergroup AF.", "eStrKey" : "LeafRef" diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/bgp.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/bgp.json index 884caca5277..0c70d99b5de 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/bgp.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/bgp.json @@ -948,6 +948,62 @@ } }, + "BGP_NEIGHBOR_AF_ROUTE_MAP_VALID": { + "sonic-route-map:sonic-route-map": { + "sonic-route-map:ROUTE_MAP_SET": { + "ROUTE_MAP_SET_LIST": [ + { + "name": "ALLOW" + } + ] + }, + "sonic-route-map:ROUTE_MAP": { + "ROUTE_MAP_LIST": [ + { + "name": "ALLOW", + "stmt_name": 1, + "route_operation": "permit" + } + ] + } + }, + "sonic-bgp-global:sonic-bgp-global": { + "sonic-bgp-global:BGP_GLOBALS": { + "BGP_GLOBALS_LIST": [ + { + "vrf_name": "default", + "local_asn": 65001 + } + ] + } + }, + "sonic-bgp-neighbor:sonic-bgp-neighbor": { + "sonic-bgp-neighbor:BGP_NEIGHBOR": { + "BGP_NEIGHBOR_LIST": [ + { + "vrf_name": "default", + "neighbor": "20.0.0.1", + "local_asn": 1, + "name": "BGP nbr 1", + "asn": 65003 + } + ] + }, + "sonic-bgp-neighbor:BGP_NEIGHBOR_AF": { + "BGP_NEIGHBOR_AF_LIST": [ + { + "vrf_name": "default", + "neighbor": "20.0.0.1", + "afi_safi": "ipv4_unicast", + "admin_status": "up", + "route_map_in": [ "ALLOW" ], + "route_map_out": [ "ALLOW" ] + } + ] + } + } + }, + "BGP_NEIGHBOR_AF_NEG_INVALID_MAP_IN": { "sonic-route-map:sonic-route-map": { "sonic-route-map:ROUTE_MAP_SET": { @@ -1306,6 +1362,62 @@ } }, + "BGP_PEERGROUP_AF_ROUTE_MAP_VALID": { + "sonic-route-map:sonic-route-map": { + "sonic-route-map:ROUTE_MAP_SET": { + "ROUTE_MAP_SET_LIST": [ + { + "name": "ALLOW" + } + ] + }, + "sonic-route-map:ROUTE_MAP": { + "ROUTE_MAP_LIST": [ + { + "name": "ALLOW", + "stmt_name": 1, + "route_operation": "permit" + } + ] + } + }, + "sonic-bgp-global:sonic-bgp-global": { + "sonic-bgp-global:BGP_GLOBALS": { + "BGP_GLOBALS_LIST": [ + { + "vrf_name": "default", + "local_asn": 65001 + } + ] + } + }, + "sonic-bgp-peergroup:sonic-bgp-peergroup": { + "sonic-bgp-peergroup:BGP_PEER_GROUP": { + "BGP_PEER_GROUP_LIST": [ + { + "vrf_name": "default", + "peer_group_name": "PG1", + "local_asn": 1, + "name": "BGP PeerGroup 01", + "asn": 65003 + } + ] + }, + "sonic-bgp-peergroup:BGP_PEER_GROUP_AF": { + "BGP_PEER_GROUP_AF_LIST": [ + { + "vrf_name": "default", + "peer_group_name": "PG1", + "afi_safi": "ipv4_unicast", + "admin_status": "up", + "route_map_in": [ "ALLOW" ], + "route_map_out": [ "ALLOW" ] + } + ] + } + } + }, + "BGP_PEERGROUP_AF_ROUTE_MAP_NOT_EXIST": { "sonic-bgp-global:sonic-bgp-global": { "sonic-bgp-global:BGP_GLOBALS": { From aae8d098820c52e32c6d3829ee063e8f057c57c9 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 19 Jun 2025 07:45:50 -0400 Subject: [PATCH 2/2] implement review comments --- src/sonic-yang-mgmt/sonic_yang_ext.py | 40 ++++++++++++++------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/sonic-yang-mgmt/sonic_yang_ext.py b/src/sonic-yang-mgmt/sonic_yang_ext.py index 42b4767068e..11962f0aa58 100644 --- a/src/sonic-yang-mgmt/sonic_yang_ext.py +++ b/src/sonic-yang-mgmt/sonic_yang_ext.py @@ -192,31 +192,34 @@ def _compileUsesClauseRefine(self, refine, model): def _compileUsesClauseList(self, model, group, name, refine): # If group doesn't have this entry, nothing to do. - if not group.get(name): + groupobj = group.get(name) + if groupobj is None: return # model doesn't have this entry type, create it as a list - if not model.get(name): + if model.get(name) is None: model[name] = [] # model has this entry type, but its not a list, convert - if not isinstance(model.get(name), list): + if not isinstance(model[name], list): model[name] = [ model[name] ] - if isinstance(group.get(name), list): - model[name].extend(group.get(name)) + if isinstance(groupobj, list): + model[name].extend(groupobj) else: - model[name].append(group.get(name)) + model[name].append(groupobj) self._compileUsesClauseRefine(refine, model[name]) - # Recursively process the yang schema looking for the "uses" clause under - # "container", "list", and "choice"/"case" nodes. Merge in the "uses" - # dictionaries for leaf and leaf-list so callers don't need to try to do - # their own "uses" processing. Remove the "uses" member when processed so - # anyone expecting it won't try to re-process. It will just look like a - # yang model that doesn't use "uses" so shouldn't cause compatibility issues. def _compileUsesClauseModel(self, module, model): + """ + Recursively process the yang schema looking for the "uses" clause under + "container", "list", and "choice"/"case" nodes. Merge in the "uses" + dictionaries for leaf and leaf-list so callers don't need to try to do + their own "uses" processing. Remove the "uses" member when processed so + anyone expecting it won't try to re-process. It will just look like a + yang model that doesn't use "uses" so shouldn't cause compatibility issues. + """ if isinstance(model, list): for item in model: self._compileUsesClauseModel(module, item) @@ -263,14 +266,13 @@ def _compileUsesClause(self): traceback.print_exc() raise e - """ - Create a map from config DB tables to container in yang model - This module name and topLevelContainer are fetched considering YANG models are - written using below Guidelines: - https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md. - """ def _createDBTableToModuleMap(self): - + """ + Create a map from config DB tables to container in yang model + This module name and topLevelContainer are fetched considering YANG models are + written using below Guidelines: + https://github.com/Azure/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md. + """ for j in self.yJson: # get module name moduleName = j['module']['@name']