From 57624f21b03cf031c5cb78ed6f84df3031245107 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 13:58:43 +0200 Subject: [PATCH 1/7] fix(sonic): honour plain arms of union leafrefs A YANG union accepts a value if any of its arms accepts it. The schema generator walked a union looking only for leafref arms and discarded the rest, and the validator then enforced what was left as if it were the whole rule. Where a union offers a plain type alongside leafrefs, a legal value was reported as a dangling reference. BGP_NEIGHBOR.local_addr is the case that surfaced: a union of inet:ip-address, leafrefs to PORT/PORTCHANNEL/LOOPBACK_INTERFACE, and a Vlan pattern. Every numbered peering the config generator emits carries a literal address there, so each one was flagged as pointing at a non-existent interface. Across nine goldens and two live config_db.json this was every leafref error reported: four in total, all false. Dropping such constraints outright would have been the cheaper fix and is the wrong one. Twenty-four of 143 leaf-level constraints come from mixed unions, and every plain arm among them is narrow -- a pattern such as inet:ip-address or Vlan[0-9]{1,4}, or a single literal escape value like default, CPU, GLOBAL or NULL. PFC_WD.ifname is a PORT leafref unioned with the one literal GLOBAL; dropping it would stop catching a port channel member naming a port that does not exist, purely because the field also spells GLOBAL. So the plain arms are kept instead. LeafrefConstraint gains plain_arms, holding each non-leafref arm as the patterns it imposes, and the validator exempts a value that an arm admits before asking whether it resolves. An arm matches when all of its patterns match, since YANG ANDs multiple pattern statements; an arm imposing no pattern at all -- a bare string, or a numeric type the generator does not render -- would admit everything, so a constraint carrying one is dropped as unenforceable rather than emitted as a rule that can never fail. No in-tree model needs that today. Patterns reach the runtime as YANG writes them. They are anchored at generation time because XSD matches a whole value while the pydantic engine searches, which would accept 999.1.1.1 for an IPv4 arm, and generation now fails unless a conformant XSD engine agrees with the runtime on every pattern emitted. pyang carries such an engine, so a dialect difference -- the Unicode category escapes in inet:ip-address, for one -- is settled as a build failure rather than surfacing as a wrong error about a real config. Nothing is translated and no runtime dependency is added; anchored pydantic agreed with XSD on all 396 probes across the twelve distinct patterns involved. Python's re could not have served here: two of those patterns do not compile under it. Should a pattern nonetheless fail to compile at runtime, the schemas and the installed pydantic disagree. That is reported as an error of its own rather than resolved either way: treating an uncompilable arm as matching would exempt every value from the reference check, leaving the validator reporting success while quietly checking less, and treating it as not matching would invent dangling references. The condition is keyed on the committed schemas rather than on the config, so it is surfaced once instead of as noise per row. Leafref errors over the same eleven artifacts go from four to zero with no other error class moving. Detection is unchanged where it matters: mutating a golden's local_addr to Ethernet999, PortChannel42 or a malformed 999.1.1.1 is still reported. This also clears the way for making the currently inert constraints reachable. BGP_NEIGHBOR.neighbor has the same union shape, so that work would otherwise have produced a false positive on every numbered neighbour instead of new true positives. Length restrictions on a plain arm are not rendered. Ignoring them only widens what an arm admits, which costs coverage rather than causing false errors. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../conductor/sonic/_generated/_leafrefs.py | 101 +++++++++- osism/tasks/conductor/sonic/validator.py | 81 +++++++- .../tasks/conductor/sonic/test_validator.py | 110 +++++++++++ tools/sonic_yang_to_pydantic.py | 173 +++++++++++++++++- 4 files changed, 458 insertions(+), 7 deletions(-) diff --git a/osism/tasks/conductor/sonic/_generated/_leafrefs.py b/osism/tasks/conductor/sonic/_generated/_leafrefs.py index 62c13d313..1341ad017 100644 --- a/osism/tasks/conductor/sonic/_generated/_leafrefs.py +++ b/osism/tasks/conductor/sonic/_generated/_leafrefs.py @@ -10,13 +10,20 @@ @dataclass(frozen=True) class LeafrefConstraint: - """A leafref from ``source_table.source_field`` to one of ``targets``.""" + """A leafref from ``source_table.source_field`` to one of ``targets``. + + ``plain_arms`` holds the non-leafref arms of a YANG union, as anchored + regexes. A union accepts a value if any arm does, so a value matching + one of these is legal without resolving to a target; an arm matches + when every pattern in it matches. + """ source_table: str source_field: str targets: Tuple[Tuple[str, str], ...] is_leaf_list: bool = False source_is_simple_key: bool = False + plain_arms: Tuple[Tuple[str, ...], ...] = () LEAFREFS: Tuple[LeafrefConstraint, ...] = ( @@ -25,6 +32,7 @@ class LeafrefConstraint: source_field="vrf_name", targets=(("VRF", "name"),), source_is_simple_key=True, + plain_arms=(("\\A(?:default)\\z",),), ), LeafrefConstraint( source_table="BGP_GLOBALS_AF", @@ -59,11 +67,35 @@ class LeafrefConstraint: ("PORTCHANNEL", "name"), ("LOOPBACK_INTERFACE", "name"), ), + plain_arms=( + ( + "\\A(?:(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?)\\z", + ), + ( + "\\A(?:((:|[0-9a-fA-F]{0,4}):)([0-9a-fA-F]{0,4}:){0,5}((([0-9a-fA-F]{0,4}:)?(:|[0-9a-fA-F]{0,4}))|(((25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])))(%[\\p{N}\\p{L}]+)?)\\z", + "\\A(?:(([^:]+:){6}(([^:]+:[^:]+)|(.*\\..*)))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?)(%.+)?)\\z", + ), + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="BGP_NEIGHBOR", source_field="neighbor", targets=(("PORT", "name"), ("PORTCHANNEL", "name")), + plain_arms=( + ( + "\\A(?:(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?)\\z", + ), + ( + "\\A(?:((:|[0-9a-fA-F]{0,4}):)([0-9a-fA-F]{0,4}:){0,5}((([0-9a-fA-F]{0,4}:)?(:|[0-9a-fA-F]{0,4}))|(((25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])))(%[\\p{N}\\p{L}]+)?)\\z", + "\\A(?:(([^:]+:){6}(([^:]+:[^:]+)|(.*\\..*)))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?)(%.+)?)\\z", + ), + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="BGP_NEIGHBOR", @@ -125,6 +157,18 @@ class LeafrefConstraint: ("PORTCHANNEL", "name"), ("LOOPBACK_INTERFACE", "name"), ), + plain_arms=( + ( + "\\A(?:(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?)\\z", + ), + ( + "\\A(?:((:|[0-9a-fA-F]{0,4}):)([0-9a-fA-F]{0,4}:){0,5}((([0-9a-fA-F]{0,4}:)?(:|[0-9a-fA-F]{0,4}))|(((25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])))(%[\\p{N}\\p{L}]+)?)\\z", + "\\A(?:(([^:]+:){6}(([^:]+:[^:]+)|(.*\\..*)))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?)(%.+)?)\\z", + ), + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="BGP_PEER_GROUP", @@ -187,6 +231,7 @@ class LeafrefConstraint: source_table="BUFFER_PG", source_field="profile", targets=(("BUFFER_PROFILE", "name"),), + plain_arms=(("\\A(?:NULL)\\z",),), ), LeafrefConstraint( source_table="BUFFER_PORT_EGRESS_PROFILE_LIST", @@ -320,6 +365,11 @@ class LeafrefConstraint: ("PORTCHANNEL", "name"), ("LOOPBACK_INTERFACE", "name"), ), + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="DHCP_SERVER_IPV4", @@ -332,6 +382,11 @@ class LeafrefConstraint: source_field="name", targets=(("MID_PLANE_BRIDGE", "bridge"),), source_is_simple_key=True, + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="DHCP_SERVER_IPV4_PORT", @@ -419,6 +474,7 @@ class LeafrefConstraint: source_table="MIRROR_SESSION", source_field="dst_port", targets=(("PORT", "name"),), + plain_arms=(("\\A(?:CPU)\\z",),), ), LeafrefConstraint( source_table="MIRROR_SESSION", @@ -435,6 +491,7 @@ class LeafrefConstraint: source_table="NEIGH", source_field="port", targets=(("PORTCHANNEL", "name"), ("PORT", "name")), + plain_arms=(("\\A(?:Vlan[0-9]+)\\z",),), ), LeafrefConstraint( source_table="NTP", @@ -446,6 +503,7 @@ class LeafrefConstraint: ("MGMT_PORT", "name"), ), is_leaf_list=True, + plain_arms=(("\\A(?:eth0)\\z",),), ), LeafrefConstraint( source_table="NTP_SERVER", @@ -484,6 +542,7 @@ class LeafrefConstraint: source_field="ifname", targets=(("PORT", "name"),), source_is_simple_key=True, + plain_arms=(("\\A(?:GLOBAL)\\z",),), ), LeafrefConstraint( source_table="PORT", @@ -526,6 +585,7 @@ class LeafrefConstraint: source_field="ifname", targets=(("PORT", "name"),), source_is_simple_key=True, + plain_arms=(("\\A(?:global)\\z",),), ), LeafrefConstraint( source_table="PORT_QOS_MAP", @@ -566,6 +626,7 @@ class LeafrefConstraint: source_table="QUEUE", source_field="ifname", targets=(("PORT", "name"),), + plain_arms=(("\\A(?:CPU)\\z",),), ), LeafrefConstraint( source_table="QUEUE", @@ -586,6 +647,11 @@ class LeafrefConstraint: ("LOOPBACK_INTERFACE", "name"), ("MGMT_PORT", "name"), ), + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="ROUTE_MAP", @@ -610,6 +676,11 @@ class LeafrefConstraint: ("PORTCHANNEL", "name"), ("LOOPBACK_INTERFACE", "name"), ), + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="ROUTE_MAP", @@ -621,6 +692,18 @@ class LeafrefConstraint: source_field="match_neighbor", targets=(("PORT", "name"), ("PORTCHANNEL", "name")), is_leaf_list=True, + plain_arms=( + ( + "\\A(?:(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?)\\z", + ), + ( + "\\A(?:((:|[0-9a-fA-F]{0,4}):)([0-9a-fA-F]{0,4}:){0,5}((([0-9a-fA-F]{0,4}:)?(:|[0-9a-fA-F]{0,4}))|(((25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])))(%[\\p{N}\\p{L}]+)?)\\z", + "\\A(?:(([^:]+:){6}(([^:]+:[^:]+)|(.*\\..*)))|((([^:]+:)*[^:]+)?::(([^:]+:)*[^:]+)?)(%.+)?)\\z", + ), + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="ROUTE_MAP", @@ -636,6 +719,7 @@ class LeafrefConstraint: source_table="ROUTE_MAP", source_field="match_src_vrf", targets=(("VRF", "name"),), + plain_arms=(("\\A(?:default)\\z",),), ), LeafrefConstraint( source_table="ROUTE_MAP", @@ -657,27 +741,36 @@ class LeafrefConstraint: source_table="ROUTE_REDISTRIBUTE", source_field="vrf_name", targets=(("VRF", "name"),), + plain_arms=(("\\A(?:default)\\z",),), ), LeafrefConstraint( source_table="SFLOW", source_field="agent_id", targets=(("PORT", "name"), ("PORTCHANNEL", "name"), ("MGMT_PORT", "name")), + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="SFLOW_SESSION", source_field="port", targets=(("PORT", "name"),), source_is_simple_key=True, + plain_arms=(("\\A(?:all)\\z",),), ), LeafrefConstraint( source_table="SRV6_MY_LOCATORS", source_field="vrf", targets=(("VRF", "name"),), + plain_arms=(("\\A(?:default)\\z",),), ), LeafrefConstraint( source_table="SRV6_MY_SIDS", source_field="decap_vrf", targets=(("VRF", "name"),), + plain_arms=(("\\A(?:default)\\z",),), ), LeafrefConstraint( source_table="SRV6_MY_SIDS", @@ -694,6 +787,7 @@ class LeafrefConstraint: source_table="SYSLOG_SERVER", source_field="vrf", targets=(("VRF", "name"),), + plain_arms=(("\\A(?:default|mgmt)\\z",),), ), LeafrefConstraint( source_table="TACPLUS", @@ -704,6 +798,11 @@ class LeafrefConstraint: ("LOOPBACK_INTERFACE", "name"), ("MGMT_PORT", "name"), ), + plain_arms=( + ( + "\\A(?:Vlan([0-9]{1,3}|[1-3][0-9]{3}|[4][0][0-8][0-9]|[4][0][9][0-4]))\\z", + ), + ), ), LeafrefConstraint( source_table="TUNNEL", diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index 1b96d10b5..6bc8ae9de 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -9,8 +9,10 @@ """ from dataclasses import dataclass, field -from typing import Any, Dict, Iterable, List, Optional +from functools import lru_cache +from typing import Annotated, Any, Dict, Iterable, List, Optional +from pydantic import StringConstraints, TypeAdapter from pydantic import ValidationError as PydValidationError from osism.tasks.conductor.sonic._generated import ( @@ -84,6 +86,19 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: ) ) + for pattern in _unusable_patterns(): + # Not a defect in the config: the committed schemas and the installed + # pydantic disagree. Reported as an error all the same, because the + # alternative is reporting success while quietly checking less. + errors.append( + ValidationError( + message=( + "generated schema is not usable with the installed pydantic: " + f"pattern {pattern!r} does not compile" + ) + ) + ) + errors.extend(_check_leafrefs(config)) return ValidationResult(valid=not errors, errors=errors, warnings=warnings) @@ -98,6 +113,11 @@ def _check_leafrefs(config: Dict[str, Any]) -> List[ValidationError]: ``config[target_table]``". Multi-target (union-of-leafref) succeeds if *any* target accepts the value. + A union may also offer non-leafref arms — ``BGP_NEIGHBOR.local_addr`` + takes a literal address as readily as an interface name. Those arms carry + no reference to resolve, so a value one of them admits is legal as it + stands and is exempt from the leafref check. + Composite-key parsing is intentionally skipped — when the source field is encoded only inside a `|`-separated row key, we can't safely split without YANG key metadata, so we only check explicit row-dict fields plus the @@ -113,6 +133,8 @@ def _check_leafrefs(config: Dict[str, Any]) -> List[ValidationError]: # references are unresolvable — flag them. for row_key, row in rows.items(): for value in _iter_leafref_values(constraint, row_key, row): + if _matches_plain_arm(constraint, value): + continue if not _value_in_any_target(value, target_keysets): errors.append( ValidationError( @@ -178,6 +200,63 @@ def _value_in_any_target(value: str, keysets: List[set]) -> bool: return any(value in ks for ks in keysets) +@lru_cache(maxsize=None) +def _pattern_adapter(pattern: str) -> Optional[TypeAdapter]: + """Compile one generated arm pattern, or ``None`` if it will not compile. + + Failing here should be impossible: the generator matches every pattern it + emits against both a conformant XSD engine and this one before committing + it. If it happens anyway, the generated schemas and the installed pydantic + disagree — an environment fault rather than anything about the config — + and :func:`_unusable_patterns` reports it. The matcher then treats the arm + as matching, so one bad pattern cannot also manufacture dangling-reference + errors on top of the incompatibility. + """ + try: + return TypeAdapter(Annotated[str, StringConstraints(pattern=pattern)]) + except Exception: + return None + + +def _matches_plain_arm(constraint: LeafrefConstraint, value: str) -> bool: + """True when a non-leafref arm of the union already admits ``value``. + + Arms are alternatives, so one matching arm is enough; within an arm YANG + requires every pattern to match. + """ + for arm in constraint.plain_arms: + if all(_matches_pattern(pattern, value) for pattern in arm): + return True + return False + + +def _matches_pattern(pattern: str, value: str) -> bool: + adapter = _pattern_adapter(pattern) + if adapter is None: + return True + try: + adapter.validate_python(value) + except PydValidationError: + return False + return True + + +def _unusable_patterns() -> List[str]: + """Generated arm patterns this pydantic cannot compile. + + Keyed on the committed schemas rather than on the config, so this reports + the same thing for every input: an incompatibility is surfaced once, and + never as noise proportional to the configuration. + """ + unusable: List[str] = [] + for constraint in LEAFREFS: + for arm in constraint.plain_arms: + for pattern in arm: + if _pattern_adapter(pattern) is None and pattern not in unusable: + unusable.append(pattern) + return unusable + + def _format_missing_message(constraint: LeafrefConstraint, value: str) -> str: targets = ", ".join(f"{t}.{f}" for t, f in constraint.targets) return ( diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 3de928d34..2ce0aa959 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -176,3 +176,113 @@ def test_unknown_table_emits_warning_not_error(): result = validate_config(config) assert any("NOT_A_REAL_TABLE" in w for w in result.warnings) assert _leafref_errors(result) == [] + + +def test_union_with_plain_type_arm_accepts_a_plain_value(): + """BGP_NEIGHBOR.local_addr is a union of `inet:ip-address`, three leafrefs + and a Vlan pattern. A literal address satisfies the first arm, so the + leafref arms must not be enforced against it.""" + config = { + "BGP_NEIGHBOR": { + "default|10.0.0.2": {"local_addr": "10.0.0.1", "asn": "65001"}, + }, + } + result = validate_config(config) + assert _leafref_errors(result) == [] + + +def test_union_with_plain_type_arm_accepts_an_ipv6_address(): + config = { + "BGP_NEIGHBOR": { + "default|fe80::2": {"local_addr": "fe80::1", "asn": "65001"}, + }, + } + result = validate_config(config) + assert _leafref_errors(result) == [] + + +def test_union_with_plain_type_arm_accepts_a_value_matching_its_pattern(): + """The Vlan arm is a bare pattern, not a leafref — SONiC comments the VLAN + leafref out — so a Vlan name resolves without any VLAN table present.""" + config = { + "BGP_NEIGHBOR": { + "default|10.0.0.2": {"local_addr": "Vlan100", "asn": "65001"}, + }, + } + result = validate_config(config) + assert _leafref_errors(result) == [] + + +def test_union_with_plain_type_arm_accepts_a_resolvable_leafref_value(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BGP_NEIGHBOR": { + "default|10.0.0.2": {"local_addr": "Ethernet0", "asn": "65001"}, + }, + } + result = validate_config(config) + assert _leafref_errors(result) == [] + + +def test_union_with_plain_type_arm_still_flags_an_unresolvable_value(): + """A value that matches no plain arm must still resolve to a target: the + plain arm exempts the values it admits, not the whole constraint.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BGP_NEIGHBOR": { + "default|10.0.0.2": {"local_addr": "Ethernet999", "asn": "65001"}, + }, + } + result = validate_config(config) + assert any( + e.table == "BGP_NEIGHBOR" and "Ethernet999" in e.message + for e in _leafref_errors(result) + ), result.errors + + +def test_union_with_literal_escape_arm_accepts_the_literal(): + """PFC_WD.ifname is a leafref to PORT unioned with the literal `GLOBAL`.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PFC_WD": {"GLOBAL": {"detection_time": "200"}}, + } + result = validate_config(config) + assert _leafref_errors(result) == [] + + +def test_union_with_literal_escape_arm_still_flags_other_values(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PFC_WD": {"Ethernet999": {"detection_time": "200"}}, + } + result = validate_config(config) + assert any( + e.table == "PFC_WD" and "Ethernet999" in e.message + for e in _leafref_errors(result) + ), result.errors + + +def test_uncompilable_arm_pattern_is_reported_not_silently_skipped(monkeypatch): + """A generated pattern the runtime cannot compile means the committed + schemas and the installed pydantic disagree. Exempting every value from + the reference check would leave the validator reporting success while + checking nothing, so the incompatibility is reported instead.""" + from osism.tasks.conductor.sonic import validator as v + + broken = v.LeafrefConstraint( + source_table="VLAN_MEMBER", + source_field="port", + targets=(("PORT", "name"),), + plain_arms=((r"\A(?:[unbalanced)\z",),), + ) + monkeypatch.setattr(v, "LEAFREFS", (broken,)) + v._pattern_adapter.cache_clear() + try: + result = v.validate_config({"PORT": {}, "VLAN_MEMBER": {"Vlan1|Ethernet0": {}}}) + assert any( + "unbalanced" in e.message or "pattern" in e.message.lower() + for e in result.errors + ), result.errors + assert not result.valid + finally: + v._pattern_adapter.cache_clear() diff --git a/tools/sonic_yang_to_pydantic.py b/tools/sonic_yang_to_pydantic.py index 564f6e529..24cfc269a 100644 --- a/tools/sonic_yang_to_pydantic.py +++ b/tools/sonic_yang_to_pydantic.py @@ -120,6 +120,13 @@ class LeafrefConstraint: ``is_leaf_list`` flags element-wise checks; ``source_is_simple_key`` is true when the source leaf is the sole `key` of its YANG list, so the row key in ConfigDB JSON directly carries the value. + + ``plain_arms`` carries the non-leafref arms of a union — a union accepts a + value if *any* arm does, so a value one of these admits is legal even + though it resolves to no target. Each arm is the tuple of YANG patterns + that arm imposes; YANG requires every pattern of an arm to match, so an + arm matches when all of its patterns do, and an *empty* arm therefore + matches everything. See :func:`extract_union_plain_arms`. """ source_table: str @@ -127,6 +134,13 @@ class LeafrefConstraint: targets: Tuple[Tuple[str, str], ...] is_leaf_list: bool = False source_is_simple_key: bool = False + plain_arms: Tuple[Tuple[str, ...], ...] = () + + @property + def is_vacuous(self) -> bool: + """True when a plain arm admits any string, making the leafref + unenforceable: every value is legal via that arm.""" + return any(len(arm) == 0 for arm in self.plain_arms) def parse_leafref_path(path: str) -> Optional[Tuple[str, str]]: @@ -182,6 +196,113 @@ def extract_leafref_targets(type_stmt) -> List[Tuple[str, str]]: return [] +def extract_union_plain_arms(type_stmt) -> List[Tuple[str, ...]]: + """Return one entry per non-leafref arm of a union, as that arm's patterns. + + A YANG `union` accepts a value if any arm accepts it, so the leafref arms + of a mixed union constrain only the values no plain arm admits. The + generator therefore has to keep the plain arms rather than discard them — + dropping them makes a legal value look like a dangling reference, and + dropping the whole constraint instead gives up checks the plain arms + barely widen (`PFC_WD.ifname` is a PORT leafref unioned with the single + literal `GLOBAL`). + + An arm is represented by the YANG patterns it imposes; YANG requires all + of them to match. An arm that restricts nothing a pattern can + express — a bare `string`, or a numeric or boolean type we do not render — + yields an empty tuple, which matches everything and so renders the whole + constraint unenforceable (:attr:`LeafrefConstraint.is_vacuous`). + `length` restrictions are not rendered either; ignoring them only widens + what an arm admits, which costs coverage rather than causing false errors. + """ + base = type_stmt.arg + if base == "leafref": + return [] + if base == "union": + arms: List[Tuple[str, ...]] = [] + for s in type_stmt.substmts: + if s.keyword == "type": + arms.extend(extract_union_plain_arms(s)) + return arms + td = getattr(type_stmt, "i_typedef", None) + if td is not None: + inner = td.search_one("type") + if inner is not None: + return extract_union_plain_arms(inner) + if base == "enumeration": + enums = [s.arg for s in type_stmt.substmts if s.keyword == "enum"] + if enums: + return [("|".join(re.escape(e) for e in enums),)] + return [()] + if base == "string": + return [tuple(s.arg for s in type_stmt.substmts if s.keyword == "pattern")] + return [()] + + +# Values the generator matches each pattern against to confirm the runtime +# engine reads it the way YANG means it. Not a proof — a smoke check wide +# enough to catch the ways the two dialects are known to diverge: unanchored +# matching, and Unicode category escapes such as `\\p{N}`. +PATTERN_PROBES = ( + "10.0.0.1", + "999.1.1.1", + "10.0.0.1%eth0", + "fe80::1", + "Ethernet0", + "Vlan100", + "Vlan4095", + "default", + "GLOBAL", + "", + " ", + "10.0.0.1\n", + "\n10.0.0.1", +) + + +def render_arm_pattern(pattern: str) -> str: + """Return *pattern* in the form the validator will match it in. + + Two things happen here rather than at runtime, so that exactly one place + knows how a YANG pattern becomes a runtime one and the two cannot drift. + + First, the pattern is anchored: XSD patterns match a whole value, while + the pydantic engine the validator uses searches, which would accept + `999.1.1.1` for an IPv4 arm. + + Second, generation fails unless both engines then agree. pyang carries a + conformant XSD matcher (libxml2 via lxml), so a dialect difference — the + Unicode escapes in `inet:ip-address`, say — is settled here as a build + failure instead of surfacing in the validator as a wrong error about a + real config. + """ + from typing import Annotated + + from pyang.types import XSDPattern # generation-time only, not a runtime dep + from pydantic import StringConstraints, TypeAdapter, ValidationError + + reference = XSDPattern(pattern, pos=None, invert_match=False) + if not reference: + raise ValueError(f"not a valid XSD pattern: {pattern!r} ({reference.error})") + + rendered = rf"\A(?:{pattern})\z" + adapter: TypeAdapter[str] = TypeAdapter( + Annotated[str, StringConstraints(pattern=rendered)] + ) + for probe in PATTERN_PROBES: + try: + adapter.validate_python(probe) + got = True + except ValidationError: + got = False + if got != reference(probe): + raise ValueError( + f"pattern {pattern!r} reads differently at runtime: XSD says " + f"{reference(probe)} for {probe!r}, the validator says {got}" + ) + return rendered + + def list_keys(list_stmt) -> List[str]: """Return the leaf names that form a YANG `list`'s key (empty if none).""" key_stmt = list_stmt.search_one("key") @@ -211,6 +332,10 @@ def collect_leafref_constraints( if not targets: continue is_simple_key = len(keys) == 1 and leaf.arg == keys[0] + plain_arms = tuple( + tuple(render_arm_pattern(p) for p in arm) + for arm in extract_union_plain_arms(type_stmt) + ) constraints.append( LeafrefConstraint( source_table=table_name, @@ -218,6 +343,7 @@ def collect_leafref_constraints( targets=tuple(targets), is_leaf_list=(leaf.keyword == "leaf-list"), source_is_simple_key=is_simple_key, + plain_arms=plain_arms, ) ) return constraints @@ -630,9 +756,14 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: Constraints that share `(source_table, source_field)` — typically because a table declares multiple `list` siblings with the same leafref leaf, e.g. INTERFACE_LIST and INTERFACE_IPPREFIX_LIST both having `name` → - PORT/name — are merged: targets are unioned and the is_leaf_list / - source_is_simple_key flags become true if any contributing constraint had - them set. + PORT/name — are merged: targets and plain arms are unioned and the + is_leaf_list / source_is_simple_key flags become true if any contributing + constraint had them set. + + Constraints left unenforceable by a plain arm that admits any string are + dropped, so the module carries no rule that cannot fail. Merging happens + first: a sibling list that widens the leaf to a bare string widens it for + the merged constraint too. """ merged: Dict[Tuple[str, str], LeafrefConstraint] = {} for c in constraints: @@ -647,6 +778,12 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: if t not in seen: seen.add(t) new_targets.append(t) + seen_arms: set = set() + new_arms: List[Tuple[str, ...]] = [] + for arm in (*existing.plain_arms, *c.plain_arms): + if arm not in seen_arms: + seen_arms.add(arm) + new_arms.append(arm) merged[key] = LeafrefConstraint( source_table=c.source_table, source_field=c.source_field, @@ -654,10 +791,12 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: is_leaf_list=existing.is_leaf_list or c.is_leaf_list, source_is_simple_key=existing.source_is_simple_key or c.source_is_simple_key, + plain_arms=tuple(new_arms), ) sorted_constraints = sorted( - merged.values(), key=lambda c: (c.source_table, c.source_field) + (c for c in merged.values() if not c.is_vacuous), + key=lambda c: (c.source_table, c.source_field), ) lines: List[str] = [] lines.append("# SPDX-License-Identifier: Apache-2.0") @@ -673,14 +812,27 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append("@dataclass(frozen=True)") lines.append("class LeafrefConstraint:") lines.append( - ' """A leafref from ``source_table.source_field`` to one of ``targets``."""' + ' """A leafref from ``source_table.source_field`` to one of ``targets``.' ) lines.append("") + lines.append( + " ``plain_arms`` holds the non-leafref arms of a YANG union, as anchored" + ) + lines.append( + " regexes. A union accepts a value if any arm does, so a value matching" + ) + lines.append( + " one of these is legal without resolving to a target; an arm matches" + ) + lines.append(" when every pattern in it matches.") + lines.append(' """') + lines.append("") lines.append(" source_table: str") lines.append(" source_field: str") lines.append(" targets: Tuple[Tuple[str, str], ...]") lines.append(" is_leaf_list: bool = False") lines.append(" source_is_simple_key: bool = False") + lines.append(" plain_arms: Tuple[Tuple[str, ...], ...] = ()") lines.append("") lines.append("") lines.append("LEAFREFS: Tuple[LeafrefConstraint, ...] = (") @@ -696,6 +848,17 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append(" is_leaf_list=True,") if c.source_is_simple_key: lines.append(" source_is_simple_key=True,") + if c.plain_arms: + arms_repr = ", ".join( + "(" + + ", ".join(repr(p) for p in arm) + + ("," if len(arm) == 1 else "") + + ")" + for arm in c.plain_arms + ) + if len(c.plain_arms) == 1: + arms_repr += "," + lines.append(f" plain_arms=({arms_repr}),") lines.append(" ),") lines.append(")") lines.append("") From da4dbc1075d86d62c0d8f19c301c7fdcb9279c39 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 13:32:34 +0200 Subject: [PATCH 2/7] fix(sonic): accept string-valued ConfigDB leaf-lists ConfigDB carries most YANG leaf-lists as a JSON array, but a handful as a single delimited string. The generator modelled every leaf-list as Optional[List[...]], so those fields were rejected outright with "Input should be a valid list". PORT.adv_speeds is one of them, and it is written for every port, which made this by far the loudest thing the validator reported: 34 errors on a spine golden out of 35, and 82 of 101 on a live config. The list of exceptions is not ours to guess. Upstream sonic-yang-mgmt keeps it in LEAF_LIST_WITH_STRING_VALUE_DICT and splits on it before handing a config to libyang, so the generator now mirrors that table verbatim, delimiters included -- NTP.src_intf separates on ';' rather than ',', which no amount of inference would have produced. A pair listed there that the vendored models define as a plain leaf rather than a leaf-list is simply never applied, as MIRROR_SESSION.src_ip currently is. Only the container shape is widened. The affected fields gain a BeforeValidator that splits a string and strips each element, and the element type is validated exactly as before, so adv_speeds="1600001", "0", "everything" or "100000,bogus" are all still reported. Splitting mirrors SONiC down to the empty case: "" yields one empty element and is rejected here as SONiC would reject it. Values already in array form pass through, since both forms reach ConfigDB. The delimiter is carried on the leafref constraints too. Resolving a reference has to read the value the same way the schema does, or a BUFFER_PORT_EGRESS_PROFILE_LIST naming "p1,p2" is reported as a single dangling profile even when both exist -- a config the schema had just accepted, failing anyway. Local evidence alone could not have settled this. Every artifact available carries adv_speeds as the string "all" -- but the config generator hard-codes that string for every port it writes, so those observations were its own output fed back, and the two live fleet configs are no more independent than the goldens are. Only one of the 53 leaf-lists in the vendored models appears in any artifact at all. The upstream table is the ground truth here, not the local corpus. Measured over 9 goldens plus 2 live configs, errors fall from 528 to 85, and every golden drops to a single remaining error. What is left is BGP_NEIGHBOR_AF.admin_status in configs predating that fix, one SYSLOG_SERVER.protocol per artifact, and MGMT_PORT.autoneg. Not addressed here: adv_speeds also carries a YANG `must` restricting `all` to appear alone, so "all,100000" is accepted although SONiC would reject it. Enforcing `must` statements is a separate capability the generator does not have for any field yet. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../conductor/sonic/_generated/_leafrefs.py | 6 +- .../conductor/sonic/_generated/_schemas.py | 201 +++++++++++------- osism/tasks/conductor/sonic/validator.py | 9 +- .../tasks/conductor/sonic/test_validator.py | 121 +++++++++++ tools/sonic_yang_to_pydantic.py | 93 +++++++- 5 files changed, 348 insertions(+), 82 deletions(-) diff --git a/osism/tasks/conductor/sonic/_generated/_leafrefs.py b/osism/tasks/conductor/sonic/_generated/_leafrefs.py index 1341ad017..dcd5532fb 100644 --- a/osism/tasks/conductor/sonic/_generated/_leafrefs.py +++ b/osism/tasks/conductor/sonic/_generated/_leafrefs.py @@ -5,7 +5,7 @@ """SONiC ConfigDB cross-table leafref constraints.""" from dataclasses import dataclass -from typing import Tuple +from typing import Optional, Tuple @dataclass(frozen=True) @@ -24,6 +24,7 @@ class LeafrefConstraint: is_leaf_list: bool = False source_is_simple_key: bool = False plain_arms: Tuple[Tuple[str, ...], ...] = () + element_delimiter: Optional[str] = None LEAFREFS: Tuple[LeafrefConstraint, ...] = ( @@ -244,6 +245,7 @@ class LeafrefConstraint: source_field="profile_list", targets=(("BUFFER_PROFILE", "name"),), is_leaf_list=True, + element_delimiter=",", ), LeafrefConstraint( source_table="BUFFER_PORT_INGRESS_PROFILE_LIST", @@ -256,6 +258,7 @@ class LeafrefConstraint: source_field="profile_list", targets=(("BUFFER_PROFILE", "name"),), is_leaf_list=True, + element_delimiter=",", ), LeafrefConstraint( source_table="BUFFER_PROFILE", @@ -504,6 +507,7 @@ class LeafrefConstraint: ), is_leaf_list=True, plain_arms=(("\\A(?:eth0)\\z",),), + element_delimiter=";", ), LeafrefConstraint( source_table="NTP_SERVER", diff --git a/osism/tasks/conductor/sonic/_generated/_schemas.py b/osism/tasks/conductor/sonic/_generated/_schemas.py index cdff4ba5b..5a5d25bbd 100644 --- a/osism/tasks/conductor/sonic/_generated/_schemas.py +++ b/osism/tasks/conductor/sonic/_generated/_schemas.py @@ -6,7 +6,33 @@ from typing import Annotated, Dict, List, Literal, Optional, Union -from pydantic import BaseModel, ConfigDict, Field, RootModel, StringConstraints +from pydantic import ( + BaseModel, + BeforeValidator, + ConfigDict, + Field, + RootModel, + StringConstraints, +) + + +def _split_delimited(delimiter: str): + """Accept a ConfigDB leaf-list written as one delimited string. + + A few leaf-lists reach ConfigDB as `"100000,50000"` rather than as a JSON + array; see LEAF_LIST_WITH_STRING_VALUE_DICT in upstream sonic-yang-mgmt. + Splitting mirrors what SONiC does before validating, down to stripping + each element, so an empty string yields one empty element and is rejected + here exactly as SONiC would reject it. Values already in array form are + passed through untouched. + """ + + def split(value): + if isinstance(value, str): + return [element.strip() for element in value.split(delimiter)] + return value + + return split # sonic-asic-sensors.yang :: sonic-asic-sensors :: ASIC_SENSORS @@ -133,16 +159,21 @@ class BgpAllowedPrefixesListRow(BaseModel): id: Optional[Annotated[int, Field(ge=0, le=4294967295)]] = None default_action: Optional[Literal["permit", "deny"]] = None prefixes_v4: Optional[ - List[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" - ), - ] + Annotated[ + List[ + Annotated[ + str, + StringConstraints( + pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" + ), + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None - prefixes_v6: Optional[List[str]] = None + prefixes_v6: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BgpAllowedPrefixesNeighListRow(BaseModel): @@ -158,16 +189,21 @@ class BgpAllowedPrefixesNeighListRow(BaseModel): neighbor_type: Optional[str] = None default_action: Optional[Literal["permit", "deny"]] = None prefixes_v4: Optional[ - List[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" - ), - ] + Annotated[ + List[ + Annotated[ + str, + StringConstraints( + pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" + ), + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None - prefixes_v6: Optional[List[str]] = None + prefixes_v6: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BgpAllowedPrefixesComListRow(BaseModel): @@ -180,16 +216,21 @@ class BgpAllowedPrefixesComListRow(BaseModel): community: Optional[str] = None default_action: Optional[Literal["permit", "deny"]] = None prefixes_v4: Optional[ - List[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" - ), - ] + Annotated[ + List[ + Annotated[ + str, + StringConstraints( + pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" + ), + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None - prefixes_v6: Optional[List[str]] = None + prefixes_v6: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BgpAllowedPrefixesNeighComListRow(BaseModel): @@ -206,16 +247,21 @@ class BgpAllowedPrefixesNeighComListRow(BaseModel): community: Optional[str] = None default_action: Optional[Literal["permit", "deny"]] = None prefixes_v4: Optional[ - List[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" - ), - ] + Annotated[ + List[ + Annotated[ + str, + StringConstraints( + pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/(([0-9])|([1-2][0-9])|(3[0-2]))( (le|ge) (([0-9])|([1-2][0-9])|(3[0-2])))?" + ), + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None - prefixes_v6: Optional[List[str]] = None + prefixes_v6: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BgpAllowedPrefixesTable( @@ -998,7 +1044,9 @@ class BufferPortEgressProfileListListRow(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) port: Optional[str] = None - profile_list: Optional[List[str]] = None + profile_list: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BufferPortEgressProfileListTable( @@ -1012,7 +1060,9 @@ class BufferPortIngressProfileListListRow(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) port: Optional[str] = None - profile_list: Optional[List[str]] = None + profile_list: Optional[ + Annotated[List[str], BeforeValidator(_split_delimited(","))] + ] = None class BufferPortIngressProfileListTable( @@ -4108,7 +4158,10 @@ class NtpGlobalRow(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) src_intf: Optional[ - List[Union[str, Annotated[str, StringConstraints(pattern="eth0")]]] + Annotated[ + List[Union[str, Annotated[str, StringConstraints(pattern="eth0")]]], + BeforeValidator(_split_delimited(";")), + ] ] = None vrf: Optional[Annotated[str, StringConstraints(pattern="mgmt|default")]] = None authentication: Optional[Literal["enabled", "disabled"]] = "disabled" @@ -4545,11 +4598,14 @@ class PortListRow(BaseModel): link_training: Optional[Annotated[str, StringConstraints(pattern="on|off")]] = None autoneg: Optional[Annotated[str, StringConstraints(pattern="on|off")]] = None adv_speeds: Optional[ - List[ - Union[ - Annotated[int, Field(ge=1, le=1600000)], - Annotated[str, StringConstraints(pattern="all")], - ] + Annotated[ + List[ + Union[ + Annotated[int, Field(ge=1, le=1600000)], + Annotated[str, StringConstraints(pattern="all")], + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None interface_type: Optional[ @@ -4581,36 +4637,39 @@ class PortListRow(BaseModel): ] ] = None adv_interface_types: Optional[ - List[ - Union[ - Literal[ - "CR", - "CR2", - "CR4", - "CR8", - "SR", - "SR2", - "SR4", - "SR8", - "LR", - "LR4", - "LR8", - "KR", - "KR4", - "KR8", - "CAUI", - "GMII", - "SFI", - "XLAUI", - "KR2", - "CAUI4", - "XAUI", - "XFI", - "XGMII", - "none", - ], - Annotated[str, StringConstraints(pattern="all")], - ] + Annotated[ + List[ + Union[ + Literal[ + "CR", + "CR2", + "CR4", + "CR8", + "SR", + "SR2", + "SR4", + "SR8", + "LR", + "LR4", + "LR8", + "KR", + "KR4", + "KR8", + "CAUI", + "GMII", + "SFI", + "XLAUI", + "KR2", + "CAUI4", + "XAUI", + "XFI", + "XGMII", + "none", + ], + Annotated[str, StringConstraints(pattern="all")], + ] + ], + BeforeValidator(_split_delimited(",")), ] ] = None mtu: Optional[Annotated[int, Field(ge=68, le=9216)]] = None diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index 6bc8ae9de..677e8e37f 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -190,7 +190,14 @@ def _iter_leafref_values( if isinstance(item, str): yield item elif isinstance(raw, str): - yield raw + # A few leaf-lists reach ConfigDB as one delimited string. The + # schema splits those; resolving the whole string as a single + # reference would report every multi-element value as dangling. + if constraint.element_delimiter: + for item in raw.split(constraint.element_delimiter): + yield item.strip() + else: + yield raw else: if isinstance(raw, str): yield raw diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 2ce0aa959..9c83daa9a 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -286,3 +286,124 @@ def test_uncompilable_arm_pattern_is_reported_not_silently_skipped(monkeypatch): assert not result.valid finally: v._pattern_adapter.cache_clear() + + +def _port_errors(result, field): + """PORT errors about one field. A leaf-list element failure extends the + path with the element index and the union arm that rejected it, so match + the field as a path segment rather than as a suffix.""" + return [ + e + for e in result.errors + if e.table == "PORT" and field in (e.path or "").split(".") + ] + + +def _rows_flagged(result, field): + return {(e.path or "").split(".")[0] for e in _port_errors(result, field)} + + +def test_string_valued_leaf_list_accepts_the_configdb_form(): + """ConfigDB carries a handful of YANG leaf-lists as a delimited string + rather than a JSON array; upstream sonic-yang-mgmt keeps the table of them + in LEAF_LIST_WITH_STRING_VALUE_DICT, and PORT.adv_speeds is one.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000", "adv_speeds": "all"}} + } + result = validate_config(config) + assert _port_errors(result, "adv_speeds") == [] + + +def test_string_valued_leaf_list_splits_multiple_elements(): + config = { + "PORT": { + "Ethernet0": {"lanes": "0", "speed": "10000", "adv_speeds": "100000,50000"}, + }, + } + result = validate_config(config) + assert _port_errors(result, "adv_speeds") == [] + + +def test_string_valued_leaf_list_strips_whitespace_around_elements(): + config = { + "PORT": { + "Ethernet0": { + "lanes": "0", + "speed": "10000", + "adv_speeds": "100000, 50000", + }, + }, + } + result = validate_config(config) + assert _port_errors(result, "adv_speeds") == [] + + +def test_string_valued_leaf_list_still_accepts_a_json_array(): + """Both forms reach ConfigDB, so neither may be rejected.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000", "adv_speeds": ["all"]}}, + } + result = validate_config(config) + assert _port_errors(result, "adv_speeds") == [] + + +def test_string_valued_leaf_list_still_validates_each_element(): + """Accepting the string form must not stop checking what is in it: the + element type is a union of uint32 1..1600000 and the literal `all`.""" + config = { + "PORT": { + "Ethernet0": { + "lanes": "0", + "speed": "10000", + "adv_speeds": "100000,nonsense", + }, + "Ethernet4": {"lanes": "4", "speed": "10000", "adv_speeds": "0"}, + }, + } + result = validate_config(config) + errors = _port_errors(result, "adv_speeds") + assert _rows_flagged(result, "adv_speeds") == {"Ethernet0", "Ethernet4"}, errors + # The complaint must be about what the elements are, not about the value + # not being a JSON array. + assert not any("valid list" in e.message for e in errors), errors + + +def test_string_valued_leaf_list_uses_the_delimiter_sonic_uses(): + """The delimiter is per field, not always a comma, so a value split on the + wrong one must not quietly validate.""" + row = {"lanes": "0", "speed": "10000"} + ok = {"PORT": {"Ethernet0": {**row, "adv_interface_types": "CR4,SR4"}}} + assert _port_errors(validate_config(ok), "adv_interface_types") == [] + wrong = {"PORT": {"Ethernet0": {**row, "adv_interface_types": "CR4;SR4"}}} + assert _port_errors(validate_config(wrong), "adv_interface_types") != [] + + +def test_string_valued_leaf_list_references_are_split_before_resolving(): + """profile_list is a leaf-list that ConfigDB carries as one delimited + string. The schema splits it; the reference check has to split it too, or + a config naming profiles that all exist is reported as dangling.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BUFFER_PROFILE": { + "p1": {"size": "0", "pool": "pool1"}, + "p2": {"size": "0", "pool": "pool1"}, + }, + "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,p2"}}, + } + result = validate_config(config) + assert [ + e + for e in _leafref_errors(result) + if e.table == "BUFFER_PORT_EGRESS_PROFILE_LIST" + ] == [], result.errors + + +def test_string_valued_leaf_list_still_flags_a_missing_element(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BUFFER_PROFILE": {"p1": {"size": "0", "pool": "pool1"}}, + "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,gone"}}, + } + errors = _leafref_errors(validate_config(config)) + assert any("gone" in e.message for e in errors), errors + assert not any("p1,gone" in e.message for e in errors), errors diff --git a/tools/sonic_yang_to_pydantic.py b/tools/sonic_yang_to_pydantic.py index 24cfc269a..e40f635d6 100644 --- a/tools/sonic_yang_to_pydantic.py +++ b/tools/sonic_yang_to_pydantic.py @@ -39,6 +39,27 @@ TYPING_NAMES = ("Annotated", "Any", "Dict", "List", "Literal", "Optional", "Union") +# ConfigDB carries most YANG leaf-lists as a JSON array, but a handful as a +# single delimited string. Upstream sonic-yang-mgmt keeps the exhaustive table +# of those exceptions and splits on it before handing a config to libyang; this +# mirrors it, so the generated schema accepts what ConfigDB actually holds. +# +# Kept verbatim from LEAF_LIST_WITH_STRING_VALUE_DICT in +# src/sonic-yang-mgmt/sonic_yang_ext.py (sonic-net/sonic-buildimage), including +# the one field that separates on ';' rather than ',': re-check it when the +# vendored YANG models are refreshed. Pairs that are a plain `leaf` in the +# vendored models rather than a `leaf-list` are simply never applied. +LEAF_LIST_STRING_DELIMITERS = { + ("MIRROR_SESSION", "src_ip"): ",", + ("NTP", "src_intf"): ";", + ("BGP_ALLOWED_PREFIXES", "prefixes_v4"): ",", + ("BGP_ALLOWED_PREFIXES", "prefixes_v6"): ",", + ("BUFFER_PORT_EGRESS_PROFILE_LIST", "profile_list"): ",", + ("BUFFER_PORT_INGRESS_PROFILE_LIST", "profile_list"): ",", + ("PORT", "adv_speeds"): ",", + ("PORT", "adv_interface_types"): ",", +} + YANG_INT_BOUNDS = { "int8": (-(2**7), 2**7 - 1), "int16": (-(2**15), 2**15 - 1), @@ -121,6 +142,10 @@ class LeafrefConstraint: true when the source leaf is the sole `key` of its YANG list, so the row key in ConfigDB JSON directly carries the value. + ``element_delimiter`` is set when ConfigDB carries this leaf-list as one + delimited string rather than a JSON array, so the references inside it can + be resolved separately instead of as one long value. + ``plain_arms`` carries the non-leafref arms of a union — a union accepts a value if *any* arm does, so a value one of these admits is legal even though it resolves to no target. Each arm is the tuple of YANG patterns @@ -135,6 +160,7 @@ class LeafrefConstraint: is_leaf_list: bool = False source_is_simple_key: bool = False plain_arms: Tuple[Tuple[str, ...], ...] = () + element_delimiter: Optional[str] = None @property def is_vacuous(self) -> bool: @@ -332,6 +358,11 @@ def collect_leafref_constraints( if not targets: continue is_simple_key = len(keys) == 1 and leaf.arg == keys[0] + delimiter = ( + LEAF_LIST_STRING_DELIMITERS.get((table_name, leaf.arg)) + if leaf.keyword == "leaf-list" + else None + ) plain_arms = tuple( tuple(render_arm_pattern(p) for p in arm) for arm in extract_union_plain_arms(type_stmt) @@ -344,6 +375,7 @@ def collect_leafref_constraints( is_leaf_list=(leaf.keyword == "leaf-list"), source_is_simple_key=is_simple_key, plain_arms=plain_arms, + element_delimiter=delimiter, ) ) return constraints @@ -527,14 +559,23 @@ def leaf_field_decl(leaf_stmt) -> str: return f" {field_name}: {annotation} = {default_repr}" -def leaf_list_field_decl(stmt) -> str: +def leaf_list_field_decl(stmt, table_name: Optional[str] = None) -> str: py = ( yang_type_to_py(stmt.search_one("type")) if stmt.search_one("type") else PyType("Any") ) field_name, alias = safe_field_name(stmt.arg) - annotation = f"Optional[List[{py.annotation}]]" + inner = f"List[{py.annotation}]" + delimiter = ( + LEAF_LIST_STRING_DELIMITERS.get((table_name, stmt.arg)) + if table_name is not None + else None + ) + if delimiter is not None: + # The elements are still validated; only the container shape is widened. + inner = f"Annotated[{inner}, BeforeValidator(_split_delimited({delimiter!r}))]" + annotation = f"Optional[{inner}]" if alias: return ( f" {field_name}: {annotation} = " f"Field(default=None, alias={alias!r})" @@ -542,6 +583,29 @@ def leaf_list_field_decl(stmt) -> str: return f" {field_name}: {annotation} = None" +# Emitted into the generated schema module when any field needs it. +SPLIT_HELPER = ''' +def _split_delimited(delimiter: str): + """Accept a ConfigDB leaf-list written as one delimited string. + + A few leaf-lists reach ConfigDB as `"100000,50000"` rather than as a JSON + array; see LEAF_LIST_WITH_STRING_VALUE_DICT in upstream sonic-yang-mgmt. + Splitting mirrors what SONiC does before validating, down to stripping + each element, so an empty string yields one empty element and is rejected + here exactly as SONiC would reject it. Values already in array form are + passed through untouched. + """ + + def split(value): + if isinstance(value, str): + return [element.strip() for element in value.split(delimiter)] + return value + + return split + +''' + + def iter_resolved_children(stmt): """Iterate the resolved children of a YANG statement (uses/grouping expanded).""" children = getattr(stmt, "i_children", None) @@ -565,13 +629,15 @@ def collect_leaves(stmt): return out -def generate_row_class(class_name: str, leaves) -> str: +def generate_row_class( + class_name: str, leaves, table_name: Optional[str] = None +) -> str: rows = [] for leaf in leaves: if leaf.keyword == "leaf": rows.append(leaf_field_decl(leaf)) elif leaf.keyword == "leaf-list": - rows.append(leaf_list_field_decl(leaf)) + rows.append(leaf_list_field_decl(leaf, table_name)) if not rows: rows = [" pass"] return ( @@ -606,14 +672,14 @@ def generate_table( for lst in lists: row_class = to_class_name(lst.arg) + "Row" leaves = collect_leaves(lst) - parts.append(generate_row_class(row_class, leaves)) + parts.append(generate_row_class(row_class, leaves, table_name)) row_classes.append(row_class) constraints.extend(collect_leafref_constraints(table_name, lst, leaves)) elif sub_containers: for sc in sub_containers: row_class = base + to_class_name(sc.arg) + "Row" leaves = collect_leaves(sc) - parts.append(generate_row_class(row_class, leaves)) + parts.append(generate_row_class(row_class, leaves, table_name)) row_classes.append(row_class) constraints.extend(collect_leafref_constraints(table_name, sc, leaves)) else: @@ -720,8 +786,13 @@ def main(argv: Optional[List[str]] = None) -> int: typing_import = ( f"from typing import {', '.join(used_typing)}\n\n" if used_typing else "" ) - pydantic_import = "from pydantic import BaseModel, ConfigDict, Field, RootModel, StringConstraints\n\n" - schema_code = HEADER_PREFIX + typing_import + pydantic_import + body + pydantic_names = ["BaseModel", "ConfigDict", "Field", "RootModel"] + if "BeforeValidator" in body: + pydantic_names.append("BeforeValidator") + pydantic_names.append("StringConstraints") + pydantic_import = f"from pydantic import {', '.join(sorted(pydantic_names))}\n\n" + helper = SPLIT_HELPER if "_split_delimited" in body else "" + schema_code = HEADER_PREFIX + typing_import + pydantic_import + helper + body out_file = output / "_schemas.py" out_file.write_text(schema_code) @@ -792,6 +863,7 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: source_is_simple_key=existing.source_is_simple_key or c.source_is_simple_key, plain_arms=tuple(new_arms), + element_delimiter=existing.element_delimiter or c.element_delimiter, ) sorted_constraints = sorted( @@ -806,7 +878,7 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append('"""SONiC ConfigDB cross-table leafref constraints."""') lines.append("") lines.append("from dataclasses import dataclass") - lines.append("from typing import Tuple") + lines.append("from typing import Optional, Tuple") lines.append("") lines.append("") lines.append("@dataclass(frozen=True)") @@ -833,6 +905,7 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append(" is_leaf_list: bool = False") lines.append(" source_is_simple_key: bool = False") lines.append(" plain_arms: Tuple[Tuple[str, ...], ...] = ()") + lines.append(" element_delimiter: Optional[str] = None") lines.append("") lines.append("") lines.append("LEAFREFS: Tuple[LeafrefConstraint, ...] = (") @@ -859,6 +932,8 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: if len(c.plain_arms) == 1: arms_repr += "," lines.append(f" plain_arms=({arms_repr}),") + if c.element_delimiter is not None: + lines.append(f" element_delimiter={c.element_delimiter!r},") lines.append(" ),") lines.append(")") lines.append("") From dd7d564ff94b2755e0636172fe240682d5ae2a34 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 09:29:06 +0200 Subject: [PATCH 3/7] fix(sonic): skip tables the vendored YANG models wrongly The models in files/sonic/yang_models/ are vendored from community SONiC, but the supported HWSKUs run Enterprise SONiC builds. Most tables agree between the two. Two do not, and for those the validator was reporting errors about values the platform considers correct: SYSLOG_SERVER.protocol community enum tcp/udp platform enum TCP/UDP/TLS, default UDP MGMT_PORT.autoneg community pattern "on|off" platform boolean, default true Both emitted values are right. The uppercase protocol comes from proto.upper() in the config generator, which is correct and must not be flipped; the four other syslog fields written alongside it -- message-type, remote-port, vrf_name, severity -- are exactly the ones the platform's model defines and the community model does not, and they pass today only because the generated models allow extra fields. Conversely the community model's port, vrf and filter leaves describe a table nothing writes. Rather than validate against a model the devices do not implement, these tables are now listed in PLATFORM_DIVERGENT_TABLES and left without a schema, joining the twenty-odd tables that already have none. They warn with the reason, so the gap reads as deliberate rather than as YANG coverage that has yet to catch up. Constraints sourced from such a table are dropped with it -- SYSLOG_SERVER.vrf names a field the platform spells vrf_name -- but the tables remain usable as leafref targets, since a ConfigDB row key carries the referenced value whichever flavour named the key leaf, and MGMT_PORT is the target of five. Vendoring the platform's own models was considered and rejected: there is no authoritative published set. The management-framework lineage in sonic-net/sonic-mgmt-common carries only four modules and no syslog model, and the complete set ships per vendor -- a search finds exactly two copies of the platform's logging model, both unofficial dumps by one uploader. SUPPORTED_HWSKUS spans two vendors anyway, so there is no single set to choose. Community YANG stays as what it is: a good approximation, authoritative for neither vendor, opted out per table where it is demonstrably wrong. With this, every committed golden validates without errors. The only errors left over the measured artifacts are 74 BGP_NEIGHBOR_AF admin_status values in two live configs that predate that fix. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../conductor/sonic/_generated/__init__.py | 9 +- .../conductor/sonic/_generated/_leafrefs.py | 6 -- .../conductor/sonic/_generated/_schemas.py | 83 ++----------------- osism/tasks/conductor/sonic/validator.py | 21 ++++- .../tasks/conductor/sonic/test_validator.py | 73 ++++++++++++++++ tools/sonic_yang_to_pydantic.py | 53 +++++++++++- 6 files changed, 156 insertions(+), 89 deletions(-) diff --git a/osism/tasks/conductor/sonic/_generated/__init__.py b/osism/tasks/conductor/sonic/_generated/__init__.py index a29b4b4bd..c1211fa54 100644 --- a/osism/tasks/conductor/sonic/_generated/__init__.py +++ b/osism/tasks/conductor/sonic/_generated/__init__.py @@ -3,6 +3,11 @@ """Generated SONiC ConfigDB schemas.""" from ._leafrefs import LEAFREFS, LeafrefConstraint -from ._schemas import TABLE_MODELS +from ._schemas import PLATFORM_DIVERGENT_TABLES, TABLE_MODELS -__all__ = ["LEAFREFS", "LeafrefConstraint", "TABLE_MODELS"] +__all__ = [ + "LEAFREFS", + "LeafrefConstraint", + "PLATFORM_DIVERGENT_TABLES", + "TABLE_MODELS", +] diff --git a/osism/tasks/conductor/sonic/_generated/_leafrefs.py b/osism/tasks/conductor/sonic/_generated/_leafrefs.py index dcd5532fb..c5d1976b2 100644 --- a/osism/tasks/conductor/sonic/_generated/_leafrefs.py +++ b/osism/tasks/conductor/sonic/_generated/_leafrefs.py @@ -787,12 +787,6 @@ class LeafrefConstraint: targets=(("FEATURE", "name"),), source_is_simple_key=True, ), - LeafrefConstraint( - source_table="SYSLOG_SERVER", - source_field="vrf", - targets=(("VRF", "name"),), - plain_arms=(("\\A(?:default|mgmt)\\z",),), - ), LeafrefConstraint( source_table="TACPLUS", source_field="src_intf", diff --git a/osism/tasks/conductor/sonic/_generated/_schemas.py b/osism/tasks/conductor/sonic/_generated/_schemas.py index 5a5d25bbd..6d2cc8f71 100644 --- a/osism/tasks/conductor/sonic/_generated/_schemas.py +++ b/osism/tasks/conductor/sonic/_generated/_schemas.py @@ -3787,30 +3787,6 @@ class MgmtInterfaceTable(RootModel[Dict[str, MgmtInterfaceListRow]]): pass -# sonic-mgmt_port.yang :: sonic-mgmt_port :: MGMT_PORT -class MgmtPortListRow(BaseModel): - model_config = ConfigDict(extra="allow", populate_by_name=True) - - name: Optional[ - Annotated[ - str, - StringConstraints( - pattern="eth([1-3][0-9]{3}|[1-9][0-9]{2}|[1-9][0-9]|[0-9])" - ), - ] - ] = None - speed: Optional[Annotated[int, Field(ge=10, le=1000)]] = None - autoneg: Optional[Annotated[str, StringConstraints(pattern="on|off")]] = None - alias: Optional[str] = None - description: Optional[str] = None - mtu: Optional[Annotated[int, Field(ge=1500, le=9216)]] = 1500 - admin_status: Optional[Literal["up", "down"]] = "up" - - -class MgmtPortTable(RootModel[Dict[str, MgmtPortListRow]]): - pass - - # sonic-mgmt_vrf.yang :: sonic-mgmt_vrf :: MGMT_VRF_CONFIG class MgmtVrfConfigVrfGlobalRow(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) @@ -5831,56 +5807,6 @@ class SuppressAsicSdkHealthEventTable( pass -# sonic-syslog.yang :: sonic-syslog :: SYSLOG_SERVER -class SyslogServerListRow(BaseModel): - model_config = ConfigDict(extra="allow", populate_by_name=True) - - server_address: Optional[ - Union[ - Union[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?" - ), - ], - str, - ], - Annotated[ - str, - StringConstraints( - min_length=1, - max_length=253, - pattern="((([a-zA-Z0-9_]([a-zA-Z0-9\\-_]){0,61})?[a-zA-Z0-9]\\.)*([a-zA-Z0-9_]([a-zA-Z0-9\\-_]){0,61})?[a-zA-Z0-9]\\.?)|\\.", - ), - ], - ] - ] = None - source: Optional[ - Union[ - Annotated[ - str, - StringConstraints( - pattern="(([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])(%[\\p{N}\\p{L}]+)?" - ), - ], - str, - ] - ] = None - port: Optional[Annotated[int, Field(ge=0, le=65535)]] = None - vrf: Optional[Union[str, Literal["default", "mgmt"]]] = None - filter: Optional[Literal["include", "exclude"]] = None - filter_regex: Optional[str] = None - protocol: Optional[Literal["tcp", "udp"]] = None - severity: Optional[ - Literal["none", "debug", "info", "notice", "warn", "error", "crit"] - ] = None - - -class SyslogServerTable(RootModel[Dict[str, SyslogServerListRow]]): - pass - - # sonic-syslog.yang :: sonic-syslog :: SYSLOG_CONFIG class SyslogConfigGlobalRow(BaseModel): model_config = ConfigDict(extra="allow", populate_by_name=True) @@ -6944,7 +6870,6 @@ class ZtpTable(RootModel[Dict[str, ZtpModeRow]]): "MCLAG_UNIQUE_IP": MclagUniqueIpTable, "MEMORY_STATISTICS": MemoryStatisticsTable, "MGMT_INTERFACE": MgmtInterfaceTable, - "MGMT_PORT": MgmtPortTable, "MGMT_VRF_CONFIG": MgmtVrfConfigTable, "MID_PLANE_BRIDGE": MidPlaneBridgeTable, "MIRROR_SESSION": MirrorSessionTable, @@ -7015,7 +6940,6 @@ class ZtpTable(RootModel[Dict[str, ZtpModeRow]]): "SWITCH_TRIMMING": SwitchTrimmingTable, "SYSLOG_CONFIG": SyslogConfigTable, "SYSLOG_CONFIG_FEATURE": SyslogConfigFeatureTable, - "SYSLOG_SERVER": SyslogServerTable, "SYSTEM_DEFAULTS": SystemDefaultsTable, "SYSTEM_PORT": SystemPortTable, "TACPLUS": TacplusTable, @@ -7044,3 +6968,10 @@ class ZtpTable(RootModel[Dict[str, ZtpModeRow]]): "XCVRD_LOG": XcvrdLogTable, "ZTP": ZtpTable, } + +# Tables deliberately left unvalidated: the vendored models describe +# them differently from the platform these configs run on. +PLATFORM_DIVERGENT_TABLES: Dict[str, str] = { + "MGMT_PORT": "the platform models autoneg as a boolean, not as `on`/`off`", + "SYSLOG_SERVER": "the platform models this table with different field names (message-type, remote-port, vrf_name) and an uppercase TCP/UDP/TLS protocol enum", +} diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index 677e8e37f..c663fb5bb 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -18,6 +18,7 @@ from osism.tasks.conductor.sonic._generated import ( LEAFREFS, LeafrefConstraint, + PLATFORM_DIVERGENT_TABLES, TABLE_MODELS, ) @@ -54,6 +55,13 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: yet modelled in upstream YANG — are reported as warnings rather than errors, so the validator does not reject otherwise-valid configurations just because YANG coverage lags. + + A few tables are absent on purpose rather than for lack of coverage: the + vendored models are community SONiC while these configs run on an + Enterprise build, and where the two disagree a model exists but describes + a different table. Those are listed in + :data:`PLATFORM_DIVERGENT_TABLES` and warn with the reason, so the + difference reads as a deliberate gap rather than as missing upstream work. """ errors: List[ValidationError] = [] warnings: List[str] = [] @@ -61,9 +69,16 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: for table_name, table_data in config.items(): model = TABLE_MODELS.get(table_name) if model is None: - warnings.append( - f"No YANG schema for table {table_name!r} (validation skipped)" - ) + reason = PLATFORM_DIVERGENT_TABLES.get(table_name) + if reason is not None: + warnings.append( + f"Table {table_name!r} is not validated: the vendored YANG " + f"disagrees with the target platform — {reason}" + ) + else: + warnings.append( + f"No YANG schema for table {table_name!r} (validation skipped)" + ) continue try: diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 9c83daa9a..72280b794 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -407,3 +407,76 @@ def test_string_valued_leaf_list_still_flags_a_missing_element(): errors = _leafref_errors(validate_config(config)) assert any("gone" in e.message for e in errors), errors assert not any("p1,gone" in e.message for e in errors), errors + + +def _warnings_for(result, table): + return [w for w in result.warnings if table in w] + + +def test_platform_divergent_table_is_not_schema_validated(): + """The vendored models are community SONiC; these devices run an + Enterprise build that models SYSLOG_SERVER with different field names and + an uppercase protocol enum. Validating one against the other only produces + false positives, so the table carries no schema.""" + config = { + "SYSLOG_SERVER": { + "192.0.2.1": { + "message-type": "log", + "protocol": "UDP", + "remote-port": "514", + "severity": "info", + "vrf_name": "mgmt", + }, + }, + } + result = validate_config(config) + assert [e for e in result.errors if e.table == "SYSLOG_SERVER"] == [], result.errors + + +def test_platform_divergent_table_says_why_it_was_skipped(): + """The warning must not read like the plain 'no YANG schema' case: here a + model exists and is deliberately not trusted.""" + result = validate_config({"SYSLOG_SERVER": {"10.0.0.1": {"protocol": "UDP"}}}) + warnings = _warnings_for(result, "SYSLOG_SERVER") + assert warnings, result.warnings + assert any("platform" in w.lower() for w in warnings), warnings + + +def test_platform_divergent_mgmt_port_accepts_the_device_value(): + """MGMT_PORT.autoneg is a boolean on the target platform; the community + model constrains it to the pattern `on|off`.""" + config = {"MGMT_PORT": {"eth0": {"autoneg": "true", "admin_status": "up"}}} + result = validate_config(config) + assert [e for e in result.errors if e.table == "MGMT_PORT"] == [], result.errors + + +def test_platform_divergent_table_drops_its_own_leafrefs(): + """SYSLOG_SERVER.vrf is a community-only field — the platform spells it + vrf_name — so the constraint sourced from it must go with the schema.""" + from osism.tasks.conductor.sonic._generated import LEAFREFS + + assert [c for c in LEAFREFS if c.source_table == "SYSLOG_SERVER"] == [] + + +def test_platform_divergent_table_still_usable_as_a_leafref_target(): + """MGMT_PORT is the target of several leafrefs. Row keys carry the value in + either flavour, so those checks stay live.""" + from osism.tasks.conductor.sonic._generated import LEAFREFS + + assert [c for c in LEAFREFS if any(t[0] == "MGMT_PORT" for t in c.targets)] + config = { + "MGMT_PORT": {"eth0": {"admin_status": "up"}}, + "MGMT_INTERFACE": {"eth99|10.0.0.1/24": {}, "eth0|10.0.0.2/24": {}}, + } + result = validate_config(config) + assert isinstance(result.errors, list) + + +def test_unmodelled_and_divergent_warnings_are_distinguishable(): + result = validate_config( + {"NOT_A_REAL_TABLE": {"x": {}}, "SYSLOG_SERVER": {"10.0.0.1": {}}} + ) + unmodelled = _warnings_for(result, "NOT_A_REAL_TABLE") + divergent = _warnings_for(result, "SYSLOG_SERVER") + assert unmodelled and divergent + assert unmodelled[0] != divergent[0] diff --git a/tools/sonic_yang_to_pydantic.py b/tools/sonic_yang_to_pydantic.py index e40f635d6..6df0a4688 100644 --- a/tools/sonic_yang_to_pydantic.py +++ b/tools/sonic_yang_to_pydantic.py @@ -60,6 +60,28 @@ ("PORT", "adv_interface_types"): ",", } +# Tables the vendored models describe differently from the platform OSISM +# targets, and which are therefore left unvalidated rather than validated +# against a model the devices do not implement. +# +# `files/sonic/yang_models/` is vendored from sonic-net/sonic-buildimage — +# community SONiC — while the supported HWSKUs run Enterprise SONiC builds +# (Broadcom lineage, via `frrcfgd` and a translib-derived schema). Most tables +# agree between the two. These do not, and validating them only manufactures +# errors about values the platform considers correct. +# +# Add a table here only with the divergence established against the platform, +# not inferred from our own generated artifacts — the config generator's output +# is not evidence about what the device expects. +PLATFORM_DIVERGENT_TABLES = { + "SYSLOG_SERVER": ( + "the platform models this table with different field names " + "(message-type, remote-port, vrf_name) and an uppercase " + "TCP/UDP/TLS protocol enum" + ), + "MGMT_PORT": ("the platform models autoneg as a boolean, not as `on`/`off`"), +} + YANG_INT_BOUNDS = { "int8": (-(2**7), 2**7 - 1), "int16": (-(2**15), 2**15 - 1), @@ -756,6 +778,7 @@ def main(argv: Optional[List[str]] = None) -> int: registry: List[Tuple[str, str]] = [] leafrefs: List[LeafrefConstraint] = [] skipped: List[Tuple[str, str]] = [] + divergent: List[str] = [] seen_tables: set = set() for path, module, container in find_table_containers(modules): @@ -772,6 +795,14 @@ def main(argv: Optional[List[str]] = None) -> int: continue seen_tables.add(table_name) + if table_name in PLATFORM_DIVERGENT_TABLES: + # No model and no constraints sourced here: both would describe a + # table the target platform implements differently. The table stays + # usable as a leafref *target*, since ConfigDB row keys carry the + # referenced value whichever flavour named the key leaf. + divergent.append(table_name) + continue + code_blocks.append(f"\n# {path.name} :: {module.arg} :: {table_name}\n{code}") registry.append((table_name, table_class)) leafrefs.extend(table_leafrefs) @@ -782,6 +813,15 @@ def main(argv: Optional[List[str]] = None) -> int: body += f' "{table_name}": {table_class},\n' body += "}\n" + body += ( + "\n# Tables deliberately left unvalidated: the vendored models describe\n" + "# them differently from the platform these configs run on.\n" + "PLATFORM_DIVERGENT_TABLES: Dict[str, str] = {\n" + ) + for table_name in sorted(divergent): + body += f" {table_name!r}: {PLATFORM_DIVERGENT_TABLES[table_name]!r},\n" + body += "}\n" + used_typing = [n for n in TYPING_NAMES if re.search(rf"\b{n}\b", body)] typing_import = ( f"from typing import {', '.join(used_typing)}\n\n" if used_typing else "" @@ -806,11 +846,20 @@ def main(argv: Optional[List[str]] = None) -> int: "# AUTO-GENERATED — DO NOT EDIT BY HAND.\n" '"""Generated SONiC ConfigDB schemas."""\n\n' "from ._leafrefs import LEAFREFS, LeafrefConstraint\n" - "from ._schemas import TABLE_MODELS\n\n" - '__all__ = ["LEAFREFS", "LeafrefConstraint", "TABLE_MODELS"]\n' + "from ._schemas import PLATFORM_DIVERGENT_TABLES, TABLE_MODELS\n\n" + "__all__ = [\n" + ' "LEAFREFS",\n' + ' "LeafrefConstraint",\n' + ' "PLATFORM_DIVERGENT_TABLES",\n' + ' "TABLE_MODELS",\n' + "]\n" ) print(f"Wrote {len(registry)} table models -> {out_file}") + if divergent: + print(f"Left {len(divergent)} table(s) unvalidated (platform divergence):") + for name in sorted(divergent): + print(f" - {name}: {PLATFORM_DIVERGENT_TABLES[name]}") print(f"Wrote {len(leafrefs)} leafref constraints -> {leafrefs_file}") if skipped: print(f"Skipped {len(skipped)} containers:") From 40bdaf835ef6db574e19148f569b0e2c4837a6b7 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 09:31:33 +0200 Subject: [PATCH 4/7] chore(sonic): pin the vendored YANG to a commit Giltfile.yaml tracked `master`, so two runs of the overlay at different times produce different models. Those models decide what the ConfigDB validator accepts, which makes an unpinned source a way for validation results to change with nothing to review. Pin it to the upstream tip at the original import instead. Comparing blob hashes against upstream turned up two things worth recording in the file, because both are traps for whoever refreshes it next. The overlay does not reproduce the committed tree. At the pinned commit 114 of the 135 vendored models are identical and 21 differ, so a refresh has to be treated as a deliberate operation -- overlay, regenerate the schemas, re-measure against the goldens -- rather than as a no-op sync. And three of the vendored models do not come from this overlay at all. Upstream keeps sonic-types.yang, sonic-extension.yang and sonic-policer.yang as Jinja templates under yang-templates/*.yang.j2 and renders them during the build; they were added by hand in a later commit. A refresh must not drop them, sonic-types.yang least of all -- the other models take their typedefs from it, admin_status among them. No behaviour changes: the vendored files are untouched and nothing invokes gilt automatically. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- Giltfile.yaml | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/Giltfile.yaml b/Giltfile.yaml index 7717b5b3f..66f2a59ec 100644 --- a/Giltfile.yaml +++ b/Giltfile.yaml @@ -1,10 +1,36 @@ --- +# Vendoring for files/sonic/yang_models/, the SONiC YANG the ConfigDB validator +# generates its Pydantic schemas from (tools/sonic_yang_to_pydantic.py). +# +# `version` is pinned rather than tracking `master`: these models decide what +# the validator accepts, so an unpinned refresh could change validation +# results with nothing to review. The pin is the upstream tip at the original +# import (2025-11-09). +# +# Two caveats, both established by comparing blob hashes against upstream: +# +# * This overlay does not reproduce the committed tree. At the pinned commit +# 114 of the 135 vendored models are identical and 21 differ, so refreshing +# is a deliberate operation: re-run the overlay, regenerate the schemas, and +# re-measure the validator against the goldens before committing the result. +# +# * Three vendored models — sonic-types.yang, sonic-extension.yang and +# sonic-policer.yang — are NOT produced by this overlay. Upstream keeps them +# as Jinja templates under src/sonic-yang-models/yang-templates/*.yang.j2 and +# renders them at build time. They were added by hand and must survive a +# refresh; sonic-types.yang in particular carries typedefs the other models +# depend on. +# +# Note also that the models are community SONiC while the supported HWSKUs run +# Enterprise SONiC builds. Where the two disagree the table is opted out via +# PLATFORM_DIVERGENT_TABLES in tools/sonic_yang_to_pydantic.py; re-check that +# list when this pin moves. giltDir: ~/.gilt/clone debug: false parallel: true repositories: - git: https://github.com/sonic-net/sonic-buildimage - version: master + version: c12ed06001bc68d3c72872142487ca30c3cc6267 sources: - src: src/sonic-yang-models/yang-models/*.yang dstDir: files/sonic/yang_models/ From 3aaefba27ad1e5d519d49fc39dca870b79360007 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 10:01:04 +0200 Subject: [PATCH 5/7] test(sonic): validate shipped ConfigDB artifacts in CI The ConfigDB validator had exactly one caller, the `osism sonic validate` command, so nothing ran it unless someone remembered to. That made it a tool rather than a gate: the schemas, the schema generator and the config generator could all drift into rejecting a config we ship and no job would notice. Run it over the committed artifacts instead. `files/sonic/config_db.json` is lifted from a real device and is the base that generated configs are layered onto; it validates with no errors, and asserting that catches a regression at PR time without NetBox or the docker-compose harness. The unit-test job already runs bare pytest over tests/unit, so no Zuul change is needed. The E2E goldens are the other artifacts worth gating on and are not on this branch yet, so the artifact list globs for them rather than naming them: they are covered as soon as that series lands. A glob that matches nothing would leave the module passing while checking nothing, so a separate test asserts the list is non-empty. Only errors are asserted on. Warnings report tables with no schema, which is a coverage signal rather than a defect signal -- SONiC ships ConfigDB tables upstream YANG does not model, and the vendored models are community SONiC while these configs come from Enterprise builds. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../sonic/test_validator_artifacts.py | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 tests/unit/tasks/conductor/sonic/test_validator_artifacts.py diff --git a/tests/unit/tasks/conductor/sonic/test_validator_artifacts.py b/tests/unit/tasks/conductor/sonic/test_validator_artifacts.py new file mode 100644 index 000000000..091b2cbee --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_validator_artifacts.py @@ -0,0 +1,57 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Run the ConfigDB validator over the artifacts the repository ships. + +``test_validator`` checks the validator's behaviour against hand-built +configs. This module checks the other direction: that real committed +artifacts pass it. That is what makes the validator a gate rather than a +command someone has to remember to run — a change to the YANG models, the +schema generator or the config generator that starts rejecting a shipped +config fails here, at PR time, without NetBox or the docker-compose harness. + +Warnings are deliberately not asserted on. They report tables with no schema, +which is a coverage signal rather than a defect signal: SONiC ships ConfigDB +tables that upstream YANG does not model, and the vendored models are +community SONiC while these configs come from Enterprise builds. +""" + +import json + +import pytest + +from osism.tasks.conductor.sonic.validator import validate_config + +from ._detection_helpers import repo_root + + +def _artifacts(): + """Every committed ConfigDB document the validator should accept. + + The base config is the one that is always present — it is lifted from a + real device and is what generated configs are layered onto. The E2E + goldens join it once that series lands; globbing rather than listing them + means they are covered the moment they appear. + """ + root = repo_root() + paths = [root / "files" / "sonic" / "config_db.json"] + paths.extend(sorted((root / "tests" / "e2e" / "golden").glob("*_config_db.json"))) + return [p for p in paths if p.exists()] + + +ARTIFACTS = _artifacts() + + +def test_there_is_something_to_validate(): + """Guard against the glob above quietly matching nothing and the module + passing while checking no artifact at all.""" + assert ARTIFACTS, "no committed ConfigDB artifacts found to validate" + + +@pytest.mark.parametrize("path", ARTIFACTS, ids=lambda p: p.name) +def test_committed_config_validates_without_errors(path): + config = json.loads(path.read_text()) + result = validate_config(config) + assert result.errors == [], "\n".join( + f"{e.table}.{e.path}: {e.message}" for e in result.errors + ) + assert result.valid From 7732a27efb9c0a3fb8d3ae2feb87b77e17eb5941 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 13:34:41 +0200 Subject: [PATCH 6/7] feat(sonic): check leafrefs carried in composite row keys ConfigDB joins a list's key values into the row key with `|`, and the referring leaf of a cross-table reference is usually one of those key components rather than a field of the row. `_check_leafrefs()` would not split such a key without YANG key metadata, so most of what the generator emits never ran: over nine E2E goldens and two live configs, 7 of 136 constraints evaluated anything at all, 64 values in total. The leafref half of the validator was close to decorative. The metadata was already parsed and thrown away. `list_keys()` reads each list's `key` statement and the result was used only to set `source_is_simple_key`; it is now emitted as TABLE_KEY_FIELDS and the validator maps row-key parts onto leaf names positionally. Lists are told apart by how many parts the key has. A table may declare several -- INTERFACE has one keyed by name and one by name plus prefix -- but no table in the vendored models declares two of the same length, so the mapping is unambiguous; 870 of 876 row keys across the measured artifacts map to exactly one list. A key matching none of them, or more than one, yields nothing rather than being mapped positionally anyway, which would fabricate a reference to check. A row key that is not a string yields nothing too: ConfigDB JSON always keys rows by string, but validate_config is a library call and a caller can build a dict that does not. That hazard predates this change -- the membership test for single-key constraints raised on such a key -- and splitting the key would have widened it to every table with key metadata. That takes the leafref pass from 7 constraints and 64 values to 17 and 303, and what comes alive is the part worth having: port channel and VLAN membership, BGP neighbour and VRF references, interface naming. Mutating a real golden so a PORTCHANNEL_MEMBER or VLAN_MEMBER names a port that does not exist is now caught. Reading key components also settled what a reference means against a partial config. A generated config is a fragment, layered onto the device's own base config, so it can name an MGMT_PORT it does not carry itself, and a union leafref can name a PORTCHANNEL while the fragment holds only PORT. A value is therefore judged against the targets the config actually carries: one that resolves nowhere is an error only when every target table is present, and otherwise is reported as a warning naming both the value and the missing tables. Warning rather than passing quietly matters, because a genuine typo lands in the same place and there is no way to tell the two apart. A target table that is present but empty still errors -- there the config does model it, so the value really is dangling. Every measured artifact validates with no leafref errors. Six existing tests were written against configs too partial to judge: four declared a BGP_NEIGHBOR without the BGP_GLOBALS its vrf_name refers to, and two named interfaces without declaring every table their union admits. Those are real gaps that only became visible once the key was read, so the fixtures were completed rather than the checks loosened. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../conductor/sonic/_generated/__init__.py | 3 +- .../conductor/sonic/_generated/_leafrefs.py | 180 +++++++++++++- osism/tasks/conductor/sonic/validator.py | 95 ++++++-- .../tasks/conductor/sonic/test_validator.py | 227 +++++++++++++++--- tools/sonic_yang_to_pydantic.py | 59 ++++- 5 files changed, 509 insertions(+), 55 deletions(-) diff --git a/osism/tasks/conductor/sonic/_generated/__init__.py b/osism/tasks/conductor/sonic/_generated/__init__.py index c1211fa54..82cdffdd9 100644 --- a/osism/tasks/conductor/sonic/_generated/__init__.py +++ b/osism/tasks/conductor/sonic/_generated/__init__.py @@ -2,12 +2,13 @@ # AUTO-GENERATED — DO NOT EDIT BY HAND. """Generated SONiC ConfigDB schemas.""" -from ._leafrefs import LEAFREFS, LeafrefConstraint +from ._leafrefs import LEAFREFS, TABLE_KEY_FIELDS, LeafrefConstraint from ._schemas import PLATFORM_DIVERGENT_TABLES, TABLE_MODELS __all__ = [ "LEAFREFS", "LeafrefConstraint", "PLATFORM_DIVERGENT_TABLES", + "TABLE_KEY_FIELDS", "TABLE_MODELS", ] diff --git a/osism/tasks/conductor/sonic/_generated/_leafrefs.py b/osism/tasks/conductor/sonic/_generated/_leafrefs.py index c5d1976b2..4b09afaa2 100644 --- a/osism/tasks/conductor/sonic/_generated/_leafrefs.py +++ b/osism/tasks/conductor/sonic/_generated/_leafrefs.py @@ -5,7 +5,7 @@ """SONiC ConfigDB cross-table leafref constraints.""" from dataclasses import dataclass -from typing import Optional, Tuple +from typing import Dict, Optional, Tuple @dataclass(frozen=True) @@ -864,3 +864,181 @@ class LeafrefConstraint: targets=(("VXLAN_TUNNEL", "name"),), ), ) + + +# Key leaves of every `list` in a table, in the order ConfigDB joins +# them into the row key with `|`. Lists are told apart by how many +# parts they have; no table declares two of the same length. +TABLE_KEY_FIELDS: Dict[str, Tuple[Tuple[str, ...], ...]] = { + "AAA": (("type",),), + "AS_PATH_SET": (("name",),), + "AUTO_TECHSUPPORT_FEATURE": (("feature_name",),), + "BGP_AGGREGATE_ADDRESS": (("aggregate-address",),), + "BGP_ALLOWED_PREFIXES": ( + ("deployment", "id"), + ("deployment", "id", "neighbor", "neighbor_type"), + ("deployment", "id", "community"), + ("deployment", "id", "neighbor", "neighbor_type", "community"), + ), + "BGP_GLOBALS": (("vrf_name",),), + "BGP_GLOBALS_AF": (("vrf_name", "afi_safi"),), + "BGP_GLOBALS_AF_AGGREGATE_ADDR": (("vrf_name", "afi_safi", "ip_prefix"),), + "BGP_GLOBALS_AF_NETWORK": (("vrf_name", "afi_safi", "ip_prefix"),), + "BGP_GLOBALS_LISTEN_PREFIX": (("vrf_name", "ip_prefix"),), + "BGP_INTERNAL_NEIGHBOR": (("neighbor",),), + "BGP_MONITORS": (("addr",),), + "BGP_NEIGHBOR": (("neighbor",), ("vrf_name", "neighbor")), + "BGP_NEIGHBOR_AF": (("vrf_name", "neighbor", "afi_safi"),), + "BGP_PEER_GROUP": (("vrf_name", "peer_group_name"),), + "BGP_PEER_GROUP_AF": (("vrf_name", "peer_group_name", "afi_safi"),), + "BGP_PEER_RANGE": (("peer_range_name",),), + "BGP_SENTINELS": (("sentinel_name",),), + "BGP_VOQ_CHASSIS_NEIGHBOR": (("neighbor",),), + "BREAKOUT_CFG": (("port",),), + "BUFFER_PG": (("port", "pg_num"),), + "BUFFER_POOL": (("name",),), + "BUFFER_PORT_EGRESS_PROFILE_LIST": (("port",),), + "BUFFER_PORT_INGRESS_PROFILE_LIST": (("port",),), + "BUFFER_PROFILE": (("name",),), + "BUFFER_QUEUE": (("port", "qindex"), ("hostname", "asic_name", "port", "qindex")), + "CABLE_LENGTH": (("name",),), + "CHASSIS_MODULE": (("name",),), + "COMMUNITY_SET": (("name",),), + "CONSOLE_PORT": (("name",),), + "COPP_GROUP": (("name",),), + "COPP_TRAP": (("name",),), + "DASH_ACL_GROUP": (("name",),), + "DASH_ACL_IN": (("eni", "stage"),), + "DASH_ACL_OUT": (("eni", "stage"),), + "DASH_ACL_RULE": (("acl_group_id", "name"),), + "DASH_APPLIANCE": (("name",),), + "DASH_ENI": (("name",),), + "DASH_QOS": (("name",),), + "DASH_ROUTE_TABLE": (("eni", "prefix"),), + "DASH_ROUTING_TYPE": (("name",),), + "DASH_VNET": (("name",),), + "DASH_VNET_MAPPING_TABLE": (("vnet", "ip_addr"),), + "DEBUG_COUNTER": (("name",),), + "DEBUG_COUNTER_DROP_REASON": (("name", "reason"),), + "DEFAULT_LOSSLESS_BUFFER_PARAMETER": (("name",),), + "DEVICE_NEIGHBOR": (("peer_name",),), + "DEVICE_NEIGHBOR_METADATA": (("name",),), + "DHCPV4_RELAY": (("name",),), + "DHCP_RELAY": (("name",),), + "DHCP_SERVER": (("ip",),), + "DHCP_SERVER_IPV4": (("name",),), + "DHCP_SERVER_IPV4_CUSTOMIZED_OPTIONS": (("name",),), + "DHCP_SERVER_IPV4_PORT": (("name", "port"),), + "DHCP_SERVER_IPV4_RANGE": (("name",),), + "DNS_NAMESERVER": (("ip",),), + "DOT1P_TO_TC_MAP": (("name",),), + "DPU": (("dpu_name",),), + "DPUS": (("dpu_name",),), + "DSCP_TO_FC_MAP": (("name",),), + "DSCP_TO_TC_MAP": (("name",),), + "EXP_TO_FC_MAP": (("name",),), + "EXTENDED_COMMUNITY_SET": (("name",),), + "FABRIC_PORT": (("name",),), + "FEATURE": (("name",),), + "FG_NHG": (("name",),), + "FG_NHG_MEMBER": (("next_hop_ip",),), + "FG_NHG_PREFIX": (("ip_prefix",),), + "FLOW_COUNTER_ROUTE_PATTERN": (("ip_prefix",), ("vrf_name", "ip_prefix")), + "GNMI_CLIENT_CERT": (("cert_cname",),), + "HEARTBEAT": (("name",),), + "HIGH_FREQUENCY_TELEMETRY_GROUP": (("profile_name", "group_name"),), + "HIGH_FREQUENCY_TELEMETRY_PROFILE": (("name",),), + "INTERFACE": (("name",), ("name", "ip-prefix")), + "LDAP_SERVER": (("hostname",),), + "LLDP_PORT": (("ifname",),), + "LOGGER": (("name",),), + "LOOPBACK_INTERFACE": (("name",), ("name", "ip-prefix")), + "LOSSLESS_TRAFFIC_PATTERN": (("name",),), + "MACSEC_PROFILE": (("name",),), + "MAP_PFC_PRIORITY_TO_QUEUE": (("name",),), + "MCLAG_DOMAIN": (("domain_id",),), + "MCLAG_INTERFACE": (("domain_id", "if_name"),), + "MCLAG_UNIQUE_IP": (("if_name",),), + "MGMT_INTERFACE": (("name", "ip_prefix"),), + "MIRROR_SESSION": (("name",),), + "MPLS_TC_TO_TC_MAP": (("name",),), + "MUX_CABLE": (("ifname",),), + "NAT_BINDINGS": (("name",),), + "NAT_POOL": (("name",),), + "NEIGH": (("port", "neighbor"),), + "NTP_KEY": (("id",),), + "NTP_SERVER": (("server_address",),), + "NVGRE_TUNNEL": (("tunnel_name",),), + "NVGRE_TUNNEL_MAP": (("tunnel_name", "tunnel_map_name"),), + "PBH_HASH": (("hash_name",),), + "PBH_HASH_FIELD": (("hash_field_name",),), + "PBH_RULE": (("table_name", "rule_name"),), + "PBH_TABLE": (("table_name",),), + "PEER_SWITCH": (("peer_switch",),), + "PFC_PRIORITY_TO_PRIORITY_GROUP_MAP": (("name",),), + "PFC_WD": (("ifname",),), + "POLICER": (("name",),), + "PORT": (("name",),), + "PORTCHANNEL": (("name",),), + "PORTCHANNEL_INTERFACE": (("name",), ("name", "ip_prefix")), + "PORTCHANNEL_MEMBER": (("name", "port"),), + "PORT_QOS_MAP": (("ifname",),), + "PORT_STORM_CONTROL": (("ifname", "storm_type"),), + "PREFIX": ( + ("name", "sequence_number", "ip_prefix", "masklength_range"), + ("name", "ip_prefix", "masklength_range"), + ), + "PREFIX_LIST": (("prefix_type", "ip-prefix"),), + "PREFIX_SET": (("name",),), + "QUEUE": (("ifname", "qindex"), ("hostname", "asic_name", "ifname", "qindex")), + "RADIUS_SERVER": (("ipaddress",),), + "REMOTE_DPU": (("dpu_name",),), + "ROUTE_MAP": (("name", "stmt_name"),), + "ROUTE_MAP_SET": (("name",),), + "ROUTE_REDISTRIBUTE": ( + ("vrf_name", "src_protocol", "dst_protocol", "addr_family"), + ), + "SCHEDULER": (("name",),), + "SFLOW_COLLECTOR": (("name",),), + "SFLOW_SESSION": (("port",),), + "SNMP_AGENT_ADDRESS_CONFIG": (("agent_ip", "port", "vrf_name"),), + "SNMP_COMMUNITY": (("name",),), + "SNMP_USER": (("name",),), + "SRV6_MY_LOCATORS": (("locator_name",),), + "SRV6_MY_SIDS": (("locator", "ip_prefix"),), + "STATIC_NAPT": (("global_ip", "ip_protocol", "global_l4_port"),), + "STATIC_NAT": (("global_ip",),), + "STATIC_ROUTE": (("prefix",), ("vrf_name", "prefix")), + "STP": (("keyleaf",),), + "STP_MST": (("keyleaf",),), + "STP_MST_INST": (("instance",),), + "STP_MST_PORT": (("inst_id", "ifname"),), + "STP_PORT": (("ifname",),), + "STP_VLAN": (("name",),), + "STP_VLAN_PORT": (("vlan-name", "ifname"),), + "SUBNET_DECAP": (("name",),), + "SUPPRESS_ASIC_SDK_HEALTH_EVENT": (("severity",),), + "SYSLOG_CONFIG_FEATURE": (("service",),), + "SYSTEM_DEFAULTS": (("name",),), + "SYSTEM_PORT": (("hostname", "asic_name", "ifname"),), + "TACPLUS_SERVER": (("ipaddress",),), + "TC_TO_DSCP_MAP": (("name",),), + "TC_TO_PRIORITY_GROUP_MAP": (("name",),), + "TC_TO_QUEUE_MAP": (("name",),), + "TELEMETRY_CLIENT": (("prefix", "name"),), + "TUNNEL": (("mux_tunnel",),), + "VDPU": (("vdpu_id",),), + "VLAN": (("name",),), + "VLAN_INTERFACE": (("name",), ("name", "ip-prefix")), + "VLAN_MEMBER": (("name", "port"),), + "VLAN_SUB_INTERFACE": (("name",), ("name", "ip-prefix")), + "VNET": (("name",),), + "VNET_ROUTE_TUNNEL": (("vnet_name", "prefix"),), + "VOQ_INBAND_INTERFACE": (("name",), ("name", "ip-prefix")), + "VRF": (("name",),), + "VXLAN_EVPN_NVO": (("name",),), + "VXLAN_TUNNEL": (("name",),), + "VXLAN_TUNNEL_MAP": (("name", "mapname"),), + "WARM_RESTART": (("module",),), + "WRED_PROFILE": (("name",),), +} diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index c663fb5bb..096e9958c 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -10,7 +10,7 @@ from dataclasses import dataclass, field from functools import lru_cache -from typing import Annotated, Any, Dict, Iterable, List, Optional +from typing import Annotated, Any, Dict, Iterable, List, Optional, Tuple from pydantic import StringConstraints, TypeAdapter from pydantic import ValidationError as PydValidationError @@ -19,6 +19,7 @@ LEAFREFS, LeafrefConstraint, PLATFORM_DIVERGENT_TABLES, + TABLE_KEY_FIELDS, TABLE_MODELS, ) @@ -114,12 +115,16 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: ) ) - errors.extend(_check_leafrefs(config)) + leafref_errors, leafref_warnings = _check_leafrefs(config) + errors.extend(leafref_errors) + warnings.extend(leafref_warnings) return ValidationResult(valid=not errors, errors=errors, warnings=warnings) -def _check_leafrefs(config: Dict[str, Any]) -> List[ValidationError]: +def _check_leafrefs( + config: Dict[str, Any], +) -> Tuple[List[ValidationError], List[str]]: """Verify every cross-table leafref reference resolves to an existing key. YANG `leafref` semantics say a leaf must point at an existing value in a @@ -133,32 +138,59 @@ def _check_leafrefs(config: Dict[str, Any]) -> List[ValidationError]: no reference to resolve, so a value one of them admits is legal as it stands and is exempt from the leafref check. - Composite-key parsing is intentionally skipped — when the source field is - encoded only inside a `|`-separated row key, we can't safely split without - YANG key metadata, so we only check explicit row-dict fields plus the - ``source_is_simple_key`` shortcut where the row key alone is the value. + Most referring values reach ConfigDB only inside the `|`-joined row key + rather than as a field of the row, so the key is split using the key + leaves the generator records in :data:`TABLE_KEY_FIELDS`. A row key whose + part count matches no declared list is left alone rather than mapped + positionally, which would invent values. + + A reference is only judged against the targets the config actually + carries. A generated config is a fragment, layered onto the device's own + base config, so it can name an `MGMT_PORT` it does not itself hold — and a + union leafref can name a `PORTCHANNEL` while the fragment carries only + `PORT`. A value that resolves nowhere is therefore an error only when every + target table is present; otherwise it is reported as unjudged, naming the + tables that were missing. A target that is present but empty is a different + matter: the config does model it, so a value missing from it is dangling. """ errors: List[ValidationError] = [] + warnings: List[str] = [] for constraint in LEAFREFS: rows = config.get(constraint.source_table) if not isinstance(rows, dict): continue target_keysets = _collect_target_keysets(config, constraint) - # If the config does not declare any of the target tables, the - # references are unresolvable — flag them. + absent = [ + table + for table, _ in constraint.targets + if not isinstance(config.get(table), dict) + ] for row_key, row in rows.items(): for value in _iter_leafref_values(constraint, row_key, row): if _matches_plain_arm(constraint, value): continue - if not _value_in_any_target(value, target_keysets): - errors.append( - ValidationError( - message=_format_missing_message(constraint, value), - path=f"{row_key}.{constraint.source_field}", - table=constraint.source_table, - ) + if _value_in_any_target(value, target_keysets): + continue + if absent: + # The value resolves in none of the targets this config + # carries, but it may well name a row of one it does not. + # Report it as unjudged rather than as dangling — and say + # so, rather than dropping it silently, since a genuine + # typo lands here too. + warnings.append( + f"{constraint.source_table}.{constraint.source_field}" + f"={value!r} is not checked: this config does not carry " + f"{', '.join(absent)}" ) - return errors + continue + errors.append( + ValidationError( + message=_format_missing_message(constraint, value), + path=f"{row_key}.{constraint.source_field}", + table=constraint.source_table, + ) + ) + return errors, warnings def _collect_target_keysets( @@ -193,9 +225,15 @@ def _iter_leafref_values( raw: Any = None if isinstance(row, dict) and constraint.source_field in row: raw = row[constraint.source_field] + elif not isinstance(row_key, str): + # ConfigDB JSON always keys rows by string, but validate_config is a + # library call and a caller can hand us a dict that does not. + raw = None elif constraint.source_is_simple_key and "|" not in row_key: # Single-key list: row key directly carries the leaf value. raw = row_key + else: + raw = _value_from_row_key(constraint, row_key) if raw is None: return @@ -218,6 +256,29 @@ def _iter_leafref_values( yield raw +def _value_from_row_key(constraint: LeafrefConstraint, row_key: str) -> Optional[str]: + """Recover this constraint's value from a `|`-joined ConfigDB row key. + + ConfigDB stores a list's key values joined with `|` in that list's key + order, so the leaf names recorded for the table map onto the parts + positionally. A table may declare several lists; they are told apart by + how many parts the key has, and a key matching none of them — or matching + more than one, which the vendored models never produce — yields nothing, + because guessing would fabricate a reference to check. + """ + variants = TABLE_KEY_FIELDS.get(constraint.source_table) + if not variants: + return None + parts = row_key.split("|") + matching = [v for v in variants if len(v) == len(parts)] + if len(matching) != 1: + return None + for name, value in zip(matching[0], parts): + if name == constraint.source_field: + return value + return None + + def _value_in_any_target(value: str, keysets: List[set]) -> bool: return any(value in ks for ks in keysets) diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 72280b794..67cc103d4 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -183,6 +183,7 @@ def test_union_with_plain_type_arm_accepts_a_plain_value(): and a Vlan pattern. A literal address satisfies the first arm, so the leafref arms must not be enforced against it.""" config = { + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, "BGP_NEIGHBOR": { "default|10.0.0.2": {"local_addr": "10.0.0.1", "asn": "65001"}, }, @@ -193,6 +194,7 @@ def test_union_with_plain_type_arm_accepts_a_plain_value(): def test_union_with_plain_type_arm_accepts_an_ipv6_address(): config = { + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, "BGP_NEIGHBOR": { "default|fe80::2": {"local_addr": "fe80::1", "asn": "65001"}, }, @@ -205,6 +207,7 @@ def test_union_with_plain_type_arm_accepts_a_value_matching_its_pattern(): """The Vlan arm is a bare pattern, not a leafref — SONiC comments the VLAN leafref out — so a Vlan name resolves without any VLAN table present.""" config = { + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, "BGP_NEIGHBOR": { "default|10.0.0.2": {"local_addr": "Vlan100", "asn": "65001"}, }, @@ -216,6 +219,7 @@ def test_union_with_plain_type_arm_accepts_a_value_matching_its_pattern(): def test_union_with_plain_type_arm_accepts_a_resolvable_leafref_value(): config = { "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, "BGP_NEIGHBOR": { "default|10.0.0.2": {"local_addr": "Ethernet0", "asn": "65001"}, }, @@ -226,9 +230,16 @@ def test_union_with_plain_type_arm_accepts_a_resolvable_leafref_value(): def test_union_with_plain_type_arm_still_flags_an_unresolvable_value(): """A value that matches no plain arm must still resolve to a target: the - plain arm exempts the values it admits, not the whole constraint.""" + plain arm exempts the values it admits, not the whole constraint. + + local_addr may name a PORT, a PORTCHANNEL or a LOOPBACK_INTERFACE, so all + three have to be present for the value to be judged rather than deferred to + the base config.""" config = { "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "LOOPBACK_INTERFACE": {"Loopback0": {}}, + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, "BGP_NEIGHBOR": { "default|10.0.0.2": {"local_addr": "Ethernet999", "asn": "65001"}, }, @@ -378,37 +389,6 @@ def test_string_valued_leaf_list_uses_the_delimiter_sonic_uses(): assert _port_errors(validate_config(wrong), "adv_interface_types") != [] -def test_string_valued_leaf_list_references_are_split_before_resolving(): - """profile_list is a leaf-list that ConfigDB carries as one delimited - string. The schema splits it; the reference check has to split it too, or - a config naming profiles that all exist is reported as dangling.""" - config = { - "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, - "BUFFER_PROFILE": { - "p1": {"size": "0", "pool": "pool1"}, - "p2": {"size": "0", "pool": "pool1"}, - }, - "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,p2"}}, - } - result = validate_config(config) - assert [ - e - for e in _leafref_errors(result) - if e.table == "BUFFER_PORT_EGRESS_PROFILE_LIST" - ] == [], result.errors - - -def test_string_valued_leaf_list_still_flags_a_missing_element(): - config = { - "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, - "BUFFER_PROFILE": {"p1": {"size": "0", "pool": "pool1"}}, - "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,gone"}}, - } - errors = _leafref_errors(validate_config(config)) - assert any("gone" in e.message for e in errors), errors - assert not any("p1,gone" in e.message for e in errors), errors - - def _warnings_for(result, table): return [w for w in result.warnings if table in w] @@ -480,3 +460,186 @@ def test_unmodelled_and_divergent_warnings_are_distinguishable(): divergent = _warnings_for(result, "SYSLOG_SERVER") assert unmodelled and divergent assert unmodelled[0] != divergent[0] + + +def test_composite_key_supplies_the_leafref_value(): + """Real configs carry no explicit `name`/`port` fields on + PORTCHANNEL_MEMBER — both values live only in the `|`-joined row key, so + the check is reachable only by splitting it.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "PORTCHANNEL_MEMBER": {"PortChannel0|Ethernet0": {}}, + } + assert _leafref_errors(validate_config(config)) == [] + + +def test_composite_key_flags_a_dangling_reference(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "PORTCHANNEL_MEMBER": {"PortChannel0|Ethernet999": {}}, + } + errors = _leafref_errors(validate_config(config)) + assert any( + e.table == "PORTCHANNEL_MEMBER" and "Ethernet999" in e.message for e in errors + ), errors + + +def test_composite_key_checks_every_component(): + """Both key components are leafrefs: the port channel and the port.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "PORTCHANNEL_MEMBER": {"PortChannel9|Ethernet0": {}}, + } + errors = _leafref_errors(validate_config(config)) + assert any( + e.table == "PORTCHANNEL_MEMBER" and "PortChannel9" in e.message for e in errors + ), errors + + +def test_composite_key_picks_the_list_matching_the_key_arity(): + """INTERFACE declares one list keyed by name and another by name plus + prefix. A two-part key must map onto the second, so the first component is + still read as the interface name.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "INTERFACE": { + "Ethernet0": {}, + "Ethernet0|10.0.0.1/31": {}, + "Ethernet999|10.0.0.3/31": {}, + }, + } + errors = [ + e for e in _leafref_errors(validate_config(config)) if e.table == "INTERFACE" + ] + assert any("Ethernet999" in e.message for e in errors), errors + assert not any("Ethernet0" in e.message for e in errors), errors + + +def test_composite_key_of_unknown_arity_is_skipped(): + """A row key with more parts than any declared list must not be guessed + at — mapping it positionally would invent values.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "PORTCHANNEL_MEMBER": {"PortChannel0|Ethernet0|extra|parts": {}}, + } + assert [ + e + for e in _leafref_errors(validate_config(config)) + if e.table == "PORTCHANNEL_MEMBER" + ] == [] + + +def test_three_part_composite_key_maps_positionally(): + """BGP_NEIGHBOR_AF is keyed vrf_name|neighbor|afi_safi; vrf_name must + resolve into BGP_GLOBALS.""" + config = { + "BGP_GLOBALS": {"default": {"local_asn": "65001"}}, + "BGP_NEIGHBOR_AF": {"nosuchvrf|10.0.0.2|ipv4_unicast": {}}, + } + errors = _leafref_errors(validate_config(config)) + assert any( + e.table == "BGP_NEIGHBOR_AF" and "nosuchvrf" in e.message for e in errors + ), errors + + +def test_leafref_warns_when_no_target_table_is_present_at_all(): + """A generated config is a fragment: it is layered onto the device's own + base config, so it can legitimately refer to a table it does not carry. + MGMT_INTERFACE names an MGMT_PORT that the base config supplies. Absent + means "cannot tell", which is a warning, not a dangling reference.""" + config = {"MGMT_INTERFACE": {"eth0|10.0.0.1/24": {}}} + result = validate_config(config) + assert [e for e in _leafref_errors(result) if e.table == "MGMT_INTERFACE"] == [] + assert any( + "MGMT_INTERFACE" in w and "MGMT_PORT" in w for w in result.warnings + ), result.warnings + + +def test_leafref_still_fails_when_the_target_table_is_present_but_empty(): + """Present-but-empty is different from absent: the config does model the + table, so a value that is not in it really is dangling. This is the line + between the two, and it is what keeps the check meaningful.""" + config = {"MGMT_PORT": {}, "MGMT_INTERFACE": {"eth0|10.0.0.1/24": {}}} + result = validate_config(config) + assert any( + e.table == "MGMT_INTERFACE" and "eth0" in e.message + for e in _leafref_errors(result) + ), result.errors + + +def test_non_string_row_key_does_not_raise(): + """`validate_config` is a library call, not only a CLI path: a caller can + hand it a dict whose row key is not a string. Reading key components must + return a result rather than propagate an AttributeError.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL_MEMBER": {123: {}, None: {}, "PortChannel0|Ethernet0": {}}, + } + result = validate_config(config) + assert isinstance(result.errors, list) + + +def test_string_valued_leaf_list_references_are_split_before_resolving(): + """profile_list is a leaf-list that ConfigDB carries as one delimited + string. The schema splits it; the reference check has to split it too, or + a config naming profiles that all exist is reported as dangling.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BUFFER_PROFILE": { + "p1": {"size": "0", "pool": "pool1"}, + "p2": {"size": "0", "pool": "pool1"}, + }, + "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,p2"}}, + } + result = validate_config(config) + assert [ + e + for e in _leafref_errors(result) + if e.table == "BUFFER_PORT_EGRESS_PROFILE_LIST" + ] == [], result.errors + + +def test_string_valued_leaf_list_still_flags_a_missing_element(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "BUFFER_PROFILE": {"p1": {"size": "0", "pool": "pool1"}}, + "BUFFER_PORT_EGRESS_PROFILE_LIST": {"Ethernet0": {"profile_list": "p1,gone"}}, + } + errors = _leafref_errors(validate_config(config)) + assert any("gone" in e.message for e in errors), errors + assert not any("p1,gone" in e.message for e in errors), errors + + +def test_multi_target_reference_is_not_judged_when_a_target_is_absent(): + """VLAN_MEMBER.port may name a PORT or a PORTCHANNEL. With PORTCHANNEL + absent from the fragment the device supplies it, so a port-channel name + cannot be called dangling merely because the other alternative is here.""" + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "VLAN": {"Vlan100": {}}, + "VLAN_MEMBER": {"Vlan100|PortChannel0": {}}, + } + result = validate_config(config) + assert [ + e for e in _leafref_errors(result) if e.table == "VLAN_MEMBER" + ] == [], result.errors + assert any( + "VLAN_MEMBER" in w and "PortChannel0" in w for w in result.warnings + ), result.warnings + + +def test_multi_target_reference_is_judged_when_every_target_is_present(): + config = { + "PORT": {"Ethernet0": {"lanes": "0", "speed": "10000"}}, + "PORTCHANNEL": {"PortChannel0": {"admin_status": "up"}}, + "VLAN": {"Vlan100": {}}, + "VLAN_MEMBER": {"Vlan100|Ethernet999": {}}, + } + errors = _leafref_errors(validate_config(config)) + assert any( + e.table == "VLAN_MEMBER" and "Ethernet999" in e.message for e in errors + ), errors diff --git a/tools/sonic_yang_to_pydantic.py b/tools/sonic_yang_to_pydantic.py index 6df0a4688..e132b19f2 100644 --- a/tools/sonic_yang_to_pydantic.py +++ b/tools/sonic_yang_to_pydantic.py @@ -359,6 +359,31 @@ def list_keys(list_stmt) -> List[str]: return key_stmt.arg.split() +def collect_table_key_fields(table_container) -> List[Tuple[str, ...]]: + """Return the key-leaf names of every `list` under one table container. + + ConfigDB joins a list's key values into the row key with `|`, so + ``BGP_NEIGHBOR_AF|default|10.0.0.2|ipv4_unicast`` carries `vrf_name`, + `neighbor` and `afi_safi` in that order. Emitting the key leaves lets the + validator recover those values positionally, which is the only way most + leafrefs are reachable at all — the referring leaf is usually a key + component and never appears as a field in the row dict. + + One entry per list, because a table may declare several with different key + arities (`INTERFACE` has one keyed by name and one by name plus prefix). + Splitting on `|` and matching on the number of parts tells them apart; no + table in the vendored models declares two lists of the same arity. + """ + out: List[Tuple[str, ...]] = [] + for node in iter_resolved_children(table_container): + if node.keyword != "list": + continue + keys = tuple(list_keys(node)) + if keys and keys not in out: + out.append(keys) + return out + + def collect_leafref_constraints( table_name: str, list_or_container, leaves ) -> List[LeafrefConstraint]: @@ -779,6 +804,7 @@ def main(argv: Optional[List[str]] = None) -> int: leafrefs: List[LeafrefConstraint] = [] skipped: List[Tuple[str, str]] = [] divergent: List[str] = [] + key_fields: Dict[str, List[Tuple[str, ...]]] = {} seen_tables: set = set() for path, module, container in find_table_containers(modules): @@ -806,6 +832,9 @@ def main(argv: Optional[List[str]] = None) -> int: code_blocks.append(f"\n# {path.name} :: {module.arg} :: {table_name}\n{code}") registry.append((table_name, table_class)) leafrefs.extend(table_leafrefs) + keys = collect_table_key_fields(container) + if keys: + key_fields[table_name] = keys body = "".join(code_blocks) body += "\n\nTABLE_MODELS: Dict[str, type[BaseModel]] = {\n" @@ -838,19 +867,20 @@ def main(argv: Optional[List[str]] = None) -> int: out_file.write_text(schema_code) leafrefs_file = output / "_leafrefs.py" - leafrefs_file.write_text(render_leafrefs_module(leafrefs)) + leafrefs_file.write_text(render_leafrefs_module(leafrefs, key_fields)) init_file = output / "__init__.py" init_file.write_text( "# SPDX-License-Identifier: Apache-2.0\n" "# AUTO-GENERATED — DO NOT EDIT BY HAND.\n" '"""Generated SONiC ConfigDB schemas."""\n\n' - "from ._leafrefs import LEAFREFS, LeafrefConstraint\n" + "from ._leafrefs import LEAFREFS, TABLE_KEY_FIELDS, LeafrefConstraint\n" "from ._schemas import PLATFORM_DIVERGENT_TABLES, TABLE_MODELS\n\n" "__all__ = [\n" ' "LEAFREFS",\n' ' "LeafrefConstraint",\n' ' "PLATFORM_DIVERGENT_TABLES",\n' + ' "TABLE_KEY_FIELDS",\n' ' "TABLE_MODELS",\n' "]\n" ) @@ -861,6 +891,7 @@ def main(argv: Optional[List[str]] = None) -> int: for name in sorted(divergent): print(f" - {name}: {PLATFORM_DIVERGENT_TABLES[name]}") print(f"Wrote {len(leafrefs)} leafref constraints -> {leafrefs_file}") + print(f"Wrote key fields for {len(key_fields)} tables") if skipped: print(f"Skipped {len(skipped)} containers:") for name, reason in skipped: @@ -870,7 +901,10 @@ def main(argv: Optional[List[str]] = None) -> int: return 0 -def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: +def render_leafrefs_module( + constraints: List[LeafrefConstraint], + key_fields: Optional[Dict[str, List[Tuple[str, ...]]]] = None, +) -> str: """Render the auto-generated `_leafrefs.py` module. Constraints that share `(source_table, source_field)` — typically because @@ -927,7 +961,7 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append('"""SONiC ConfigDB cross-table leafref constraints."""') lines.append("") lines.append("from dataclasses import dataclass") - lines.append("from typing import Optional, Tuple") + lines.append("from typing import Dict, Optional, Tuple") lines.append("") lines.append("") lines.append("@dataclass(frozen=True)") @@ -986,6 +1020,23 @@ def render_leafrefs_module(constraints: List[LeafrefConstraint]) -> str: lines.append(" ),") lines.append(")") lines.append("") + lines.append("") + lines.append("# Key leaves of every `list` in a table, in the order ConfigDB joins") + lines.append("# them into the row key with `|`. Lists are told apart by how many") + lines.append("# parts they have; no table declares two of the same length.") + lines.append("TABLE_KEY_FIELDS: Dict[str, Tuple[Tuple[str, ...], ...]] = {") + all_key_fields = key_fields or {} + for table_name in sorted(all_key_fields): + variants = all_key_fields[table_name] + rendered = ", ".join( + "(" + ", ".join(repr(k) for k in v) + ("," if len(v) == 1 else "") + ")" + for v in variants + ) + if len(variants) == 1: + rendered += "," + lines.append(f" {table_name!r}: ({rendered}),") + lines.append("}") + lines.append("") return "\n".join(lines) From cba79fbd8bb3fd096f185f1362f566c621b66939 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Wed, 26 Aug 2026 13:34:59 +0200 Subject: [PATCH 7/7] docs: add notes on SONiC ConfigDB validation Nothing explains how `osism sonic validate` is put together or, more importantly, what a clean result does not mean. Working that out from the code costs hours, because the significant parts are absent from it: what is deliberately not checked, and that the vendored YANG models describe a different SONiC to the one the switches run. Brief notes rather than a reference. The pipeline from vendored YANG through the committed generated schemas, the community-vs-Enterprise caveat and how tables opt out of it, a list of the gaps a clean result hides, and the two ConfigDB shapes the generator has to accommodate that the YANG does not suggest. The two halves of the validator are quantified rather than described, because they pull very different weight and "most constraints never fire" reads as though the whole exercise were ceremony. Over nine E2E goldens and two live configs, per-field type validation covers 34 tables, 911 rows and 5857 field values, while the cross-table leafref pass evaluates 17 of 136 constraints and 303 values. Type validation is the broader half and has found every error class so far; the leafref pass is narrower but covers what type checking cannot. The gaps are stated with the same specificity: a reference is judged only against the tables the config carries, since a generated config is a fragment the device layers onto its own base config; and leafrefs whose XPath the generator could not parse -- relative paths and predicated ones -- are not among the generated constraints at all. Also records, because it has produced wrong conclusions more than once, that a config_db.json from a switch OSISM manages is not evidence about what the device expects: the config generator wrote those values. Linked from README.md under a new Documentation heading, so the next doc has somewhere to go. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- README.md | 6 ++ docs/sonic-config-validation.md | 150 ++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) create mode 100644 docs/sonic-config-validation.md diff --git a/README.md b/README.md index 1c9cea93a..b93c9ad0d 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,12 @@ [![PyPi license](https://badgen.net/pypi/license/osism/)](https://pypi.org/project/osism/) [![Documentation](https://img.shields.io/static/v1?label=&message=documentation&color=blue)](https://osism.tech/docs/references/cli) +## Documentation + +- [SONiC ConfigDB validation](docs/sonic-config-validation.md) — how + `osism sonic validate` is built, what it does not check, and the caveat that + the vendored YANG models are a different SONiC flavour to the devices. + ## Running unit tests Install development dependencies and run the full unit test suite: diff --git a/docs/sonic-config-validation.md b/docs/sonic-config-validation.md new file mode 100644 index 000000000..1e27f196b --- /dev/null +++ b/docs/sonic-config-validation.md @@ -0,0 +1,150 @@ +# SONiC ConfigDB validation + +`osism sonic validate` checks a SONiC `config_db.json` against schemas derived +from the SONiC YANG models. These are notes on the parts that are not apparent +from the code — above all what the validator does *not* check, and the one +caveat worth knowing before trusting a result. + +## How it fits together + +``` +files/sonic/yang_models/*.yang vendored upstream YANG (see Giltfile.yaml) + │ + │ tools/sonic_yang_to_pydantic.py run by hand, needs pyang + black + ▼ +osism/tasks/conductor/sonic/_generated/ committed, never edited by hand + │ _schemas.py one Pydantic model per ConfigDB table + │ _leafrefs.py cross-table reference constraints + ▼ +osism/tasks/conductor/sonic/validator.py +``` + +The generated code is committed so the runtime needs no YANG tooling — pydantic +is the only dependency. `pyang` and `black` are needed only to regenerate. + +Two callers: the `osism sonic validate` command, and a unit test that runs the +validator over every ConfigDB artifact the repository ships, so a change that +starts rejecting one fails at PR time. + +## The caveat: the models are a different SONiC to the devices + +`files/sonic/yang_models/` is vendored from **community** SONiC +(`sonic-net/sonic-buildimage`). The switches in `SUPPORTED_HWSKUS` run +**Enterprise** SONiC builds (Broadcom lineage — the same reason the BGP tables +are `BGP_GLOBALS*` and the consumer is `frrcfgd` rather than `bgpcfgd`). + +Most tables are identical between the two — `sonic-bgp-common.yang` is +byte-for-byte the same file. Some are not, and there the community model +describes a table the devices do not implement: + +| field | community model | what the devices run | +|--------------------------|-------------------|------------------------| +| `SYSLOG_SERVER.protocol` | enum `tcp`/`udp` | enum `TCP`/`UDP`/`TLS` | +| `MGMT_PORT.autoneg` | pattern `on\|off` | boolean | + +Tables like these are listed in `PLATFORM_DIVERGENT_TABLES` in the generator. +They get no schema and are reported as a warning naming the reason, instead of +producing errors about values that are correct. + +Vendoring the devices' own models instead is not currently possible: there is +no authoritative published Enterprise model set. The management-framework +lineage in `sonic-net/sonic-mgmt-common` carries only a handful of modules, the +full sets ship inside vendor distributions, and `SUPPORTED_HWSKUS` spans two +vendors anyway. Treat community YANG as what it is — a good approximation that +is authoritative for neither vendor. + +**When a new error looks like a false positive**, check the field against what +the *device* expects. Do not check it against our own generated configs or +against a `config_db.json` pulled from a switch OSISM manages: the config +generator wrote those values, so they only tell you what we already emit. This +has produced wrong conclusions more than once. + +## What it actually buys you + +Worth calibrating before reading the list of gaps below, because the two halves +of the validator pull very different weight. Measured over nine E2E goldens and +two live configs: + +| check | coverage on those artifacts | +|---------------------------|----------------------------------------| +| per-field type validation | 34 tables, 911 rows, 5857 field values | +| cross-table leafref pass | 17 of 136 constraints, 303 values | + +Type validation is the broader of the two, and every error class found so far +has come from it — an enum written in the wrong case, a leaf-list emitted in a +shape the schema did not accept, and so on. The leafref pass is narrower but +covers what type checking cannot: that port channel and VLAN membership, BGP +neighbour and VRF references and interface names actually point at something. +Most of the remaining constraints are for tables no artifact here contains. + +## What the validator does not check + +Do not read a clean result as "this config is correct". Known gaps: + +- **Tables with no schema pass untouched.** Upstream YANG does not model every + ConfigDB table, and the Enterprise-only tables are largely absent from the + community models. They are reported as warnings. Warnings are a coverage + signal, not a defect signal — a gate should key on errors only. +- **Unknown fields are allowed.** Row models are generated with + `extra="allow"`, so a misspelled or platform-specific field is never + reported. Several fields written for `SYSLOG_SERVER` are invisible this way. +- **`must` statements are not modelled at all,** for any field. `adv_speeds` + carries one restricting `all` to appear alone, for instance, and `"all,100000"` + validates here although SONiC would reject it. +- **A reference is only judged against the tables the config carries.** A + generated config is a fragment — the device layers it onto its own base + config — so naming an `MGMT_PORT` it does not itself contain is legitimate, + and a union leafref may name a `PORTCHANNEL` while the fragment holds only + `PORT`. A value that resolves nowhere is an error only when *every* target + table is present; otherwise it is reported as a warning naming the value and + the missing tables. A typo lands there too and cannot be told apart, which is + why it warns rather than passing silently. A target table that is *present + but empty* still errors, since there the config does model it. +- **Leafrefs the generator could not resolve are absent entirely.** The XPath + parser gives up on relative paths (`../..`) and on any path carrying a + predicate — `BGP_NEIGHBOR_AF.neighbor` is both — so those references are not + among the generated constraints and nothing checks them here. +- **Patterns are matched unanchored in `_schemas.py`.** YANG patterns are XSD + regexes and match a whole value, but the generated `pattern=` constraints are + searched, so a valid value with junk around it passes. (The leafref side does + anchor.) +- **Nothing validates during a sync.** The validator does not run when a config + is generated or pushed. + +## ConfigDB shapes the schemas have to accommodate + +Two places where ConfigDB does not look like the YANG suggests, both handled in +the generator and worth knowing before changing it: + +- **Some leaf-lists are a single delimited string,** not a JSON array — + `adv_speeds` is `"100000,50000"`. The exceptions are not guessable; upstream + keeps the list in `LEAF_LIST_WITH_STRING_VALUE_DICT` + (`src/sonic-yang-mgmt/sonic_yang_ext.py`) and the generator mirrors it as + `LEAF_LIST_STRING_DELIMITERS`. Note one field separates on `;`. +- **A union may mix leafrefs with plain types.** `BGP_NEIGHBOR.local_addr` + takes a literal IP *or* an interface name. A value the plain arms admit is + legal even though it resolves to no table, so the leafref check has to let it + through — see `plain_arms` on the generated constraints. + +## Regenerating + +Needed whenever the vendored YANG or the generator changes. `_generated/` is +committed; do not hand-edit it. + +``` +pip install pyang black +python tools/sonic_yang_to_pydantic.py +``` + +Generation fails rather than emitting a union arm pattern that the runtime +would read differently from XSD: each is checked against pyang's XSD matcher +first. The `pattern=` constraints in `_schemas.py` do not go through that check +— that is the unanchored-matching gap noted above. + +## Refreshing the vendored YANG + +`Giltfile.yaml` pins the upstream commit, and the header there records two +traps: the overlay does not reproduce the committed tree, and three models are +rendered from Jinja templates upstream so the overlay does not produce them at +all. Read it before refreshing, and regenerate the schemas and re-check the +shipped artifacts afterwards.