fix(sonic): make the ConfigDB validator usable as a gate - #2626
Open
ideaship wants to merge 7 commits into
Open
Conversation
ideaship
force-pushed
the
sonic-validator-union-leafref
branch
from
August 26, 2026 10:53
4b4fc47 to
9d3ec68
Compare
ideaship
force-pushed
the
sonic-validator-union-leafref
branch
from
August 26, 2026 11:41
20b6881 to
3d3a9e9
Compare
ideaship
marked this pull request as ready for review
August 26, 2026 11:49
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="osism/tasks/conductor/sonic/validator.py" line_range="273-281" />
<code_context>
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.
+
+ The generator matches every pattern it emits against a conformant XSD
+ engine, so a pattern that fails here means the generated module and the
+ installed pydantic disagree. Callers treat that as "arm matches" rather
+ than report a reference we cannot actually judge.
+ """
+ try:
+ return TypeAdapter(Annotated[str, StringConstraints(pattern=pattern)])
</code_context>
<issue_to_address>
**issue (bug_risk):** When Pydantic cannot compile a generated plain-arm pattern, `_pattern_adapter` returns `None`, `_matches_pattern` treats that as a successful match, and the validator exempts every value from the leafref check. This silently converts a runtime incompatibility into skipped validation instead of reporting an error.
**Triggers:** When the generated artifact is run with a Pydantic/regex engine that rejects one of the emitted patterns.
**Suggested fix:** Fail validation or raise a clear compatibility error when the adapter cannot be constructed; do not interpret an uncompiled pattern as matching every value.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: osism/tasks/conductor/sonic/validator.py:281
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 <luethi@osism.tech>
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 <luethi@osism.tech>
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 <luethi@osism.tech>
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 <luethi@osism.tech>
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 <luethi@osism.tech>
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 <luethi@osism.tech>
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 <luethi@osism.tech>
ideaship
force-pushed
the
sonic-validator-union-leafref
branch
from
August 26, 2026 12:00
3d3a9e9 to
cba79fb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ConfigDB validator worked but was not usable as a gate: nothing ran it,
and it reported 35–46 errors per device of which almost none were defects.
This closes that gap. Every committed artifact now validates with no errors,
and a unit test keeps it that way.
What the numbers were measured against
Eleven ConfigDB documents, none of which are in this branch, so the figures
below cannot be reproduced from it alone:
tests/e2e/golden/*_config_db.jsonfrom the SONiC E2E series, taken atits tip — the series first adds them here:
config_db.jsonfrom a live fleet, which are not public.What is in this branch and gated on is
files/sonic/config_db.json, the baseconfig lifted from a real device. The new test globs for the goldens as well,
so they come under the gate automatically once that series lands.
What was wrong
Five classes of error. Only two were ours to fix; the rest were the validator
being wrong about correct configuration.
PORT.adv_speeds—"all"against a list schemaBGP_NEIGHBOR.local_addrunion leafrefSYSLOG_SERVER.protocol—"UDP"vstcp/udpMGMT_PORT.autoneg—"true"vson|offBGP_NEIGHBOR_AF.admin_statusUnion leafrefs. A YANG
unionaccepts a value if any arm does. Thegenerator kept only the leafref arms and then required a match, so the literal
IP in
BGP_NEIGHBOR.local_addrlooked like a dangling reference. The plainarms are now carried onto the constraint and exempt the values they admit.
Dropping such constraints instead — the cheaper fix — would have stopped
checking
PFC_WD.ifnameagainstPORTmerely because the field also acceptsthe literal
GLOBAL.String-valued leaf-lists. ConfigDB carries a few leaf-lists as one
delimited string rather than a JSON array. The exceptions are not guessable, so
the generator mirrors the table upstream
sonic-yang-mgmtkeeps for exactlythis, delimiters included — one field separates on
;.The vendored models are a different SONiC.
files/sonic/yang_models/comesfrom community SONiC; the HWSKUs in
SUPPORTED_HWSKUSrun Enterprise builds.Most tables agree —
sonic-bgp-common.yangis byte-identical — but where theydo not, the community model describes a table the devices do not implement.
Those tables are now listed in
PLATFORM_DIVERGENT_TABLES, get no schema, andwarn with the reason.
proto.upper()in the config generator is correct andmust not be flipped.
Vendoring the devices' own models was considered and rejected: no authoritative
Enterprise set is published, and
SUPPORTED_HWSKUSspans two vendors anyway.Making the leafref half do something
Most generated constraints never ran, because the referring leaf is usually a
component of the
|-joined row key rather than a field of the row. The keymetadata was already parsed and discarded; it is now emitted, and row-key parts
map onto leaf names positionally. Lists are told apart by key arity, which is
unambiguous — no table declares two lists of the same length.
What comes alive is worth having: port channel and VLAN membership, BGP
neighbour and VRF references, interface naming. Mutating a golden so a
PORTCHANNEL_MEMBERorVLAN_MEMBERnames a port that does not exist is nowcaught.
This also exposed a distinction the checker did not draw. A generated config is
a fragment layered onto the device's own base config, so it can legitimately
name an
MGMT_PORTit does not carry. A target table that is wholly absentnow warns; one that is present but empty still errors, because there the
config does model the table and the value really is dangling.
Result
Errors over the eleven measured artifacts fall from 528 to 74, and all nine
goldens validate with zero errors. What remains is
BGP_NEIGHBOR_AF.admin_statusin the two live configs, which predate that fix.The shipped base config validates clean too, which is what the new test
asserts today.
Also here: the vendored YANG is pinned (it tracked
master, so the modelsbacking the validator could change with nothing to review), and
docs/sonic-config-validation.mdrecords how the pipeline fits together and —more usefully — what a clean result does not mean.
Notes for review
PLATFORM_DIVERGENT_TABLESrather than validating them against a model thedevices do not implement, and treating an absent target table as unjudgeable
rather than dangling.
open key-reference work and will conflict in three small hunks; I have run
that rebase and the two mechanisms are complementary, not overlapping —
KEY_PREFIX_REFScovers a leafref whose XPath the generator cannot parse atall, which this cannot reach. No double-reporting:
also covers the new key-splitting path.
🤖 Generated with Claude Code