feat(sonic): validate BGP_NEIGHBOR_AF key references - #2619
Open
ideaship wants to merge 2 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
berendt
approved these changes
Aug 27, 2026
ideaship
force-pushed
the
sonic-validator-af-leafref
branch
from
August 30, 2026 15:06
f9da38d to
134514d
Compare
BGP_NEIGHBOR_AF.neighbor is a YANG leafref into BGP_NEIGHBOR restricted
to the same VRF, so the vrf_name|neighbor prefix of an AF row key must
name an existing neighbor. Nothing checked that. An AF row could
activate an address family for a peer with no BGP_NEIGHBOR entry, while
the neighbor that does exist was left with no address family at all --
a session that comes up and exchanges nothing.
The generated constraint table cannot express it. The leafref path is
both relative and predicated:
../../../BGP_NEIGHBOR/BGP_NEIGHBOR_LIST[vrf_name=current()/../vrf_name]/neighbor
and parse_leafref_path() in tools/sonic_yang_to_pydantic.py returns None
for either shape, so no constraint is emitted for this leaf. Composite
row keys are otherwise covered: of the nine constraints the generator
does emit for this table, vrf_name is read out of the row key and
checked against BGP_GLOBALS. Only the component that decides whether the
row names a real peer goes unchecked.
KEY_PREFIX_REFS is therefore hand-maintained and lives beside the
validator logic rather than in _generated/, which is marked do-not-edit.
Rows are skipped when their key has too few components, or when the key
is not a string: both are malformed rows that the row schema already
reports, and reporting them here too would turn one defect into two.
Measured over the E2E goldens and two config_db.json from a live fleet,
none of which are in tree: all 8 address-family rows the goldens carried
before the keying fix are flagged, none of the 12 they carry after it
are, and none of the 74 fleet rows are. The check separates the two
shapes it exists to distinguish.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
_iter_leafref_values() evaluates "|" not in row_key for tables whose list has a single key. A non-string row key makes that membership test raise TypeError, so validate_config() propagates an exception instead of returning a ValidationResult -- the one thing a validator should never do, since the caller cannot tell a malformed config from a broken validator. This started as a guard on that membership test. main has since grown a broader one: _iter_leafref_values() yields nothing for a non-string row key before the test is reached, which covers every shape rather than that one call. The guard is dropped here as redundant, leaving the test, so the behaviour stays pinned either way. JSON object keys are always strings, so the case is out of reach for a config read from a file. It is reachable through the in-memory dict the function also accepts, and through any loader producing non-string keys. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
sonic-validator-af-leafref
branch
from
August 31, 2026 08:48
134514d to
c864541
Compare
sourcery-ai
Bot
dismissed
their stale review
August 31, 2026 08:49
Sourcery withdrew this approval because the latest commits introduced blocking findings.
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
3 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.
Problem
BGP_NEIGHBOR_AF.neighboris a YANG leafref intoBGP_NEIGHBOR, restricted tothe same VRF by an XPath predicate:
—
sonic-bgp-neighbor.yang#L138-L146So the
vrf_name|neighborprefix of an AF row key must name an existingneighbor. Nothing checks that. An AF row can activate an address family for a
peer that has no
BGP_NEIGHBORentry, while the neighbor that does exist isleft with no address family at all — a session that comes up and exchanges
nothing.
Why the generated table does not cover it
The validator now splits
|-joined row keys using the key leaves the generatorrecords in
TABLE_KEY_FIELDS, and it does validate part of this very key: thevrf_namecomponent is checked againstBGP_GLOBALS. What it cannot check isneighbor, because no constraint exists for it —parse_leafref_path()returnsNonefor relative paths (../…) andfor any path carrying an XPath predicate (
[…]), and this leafref is both. So_generated/_leafrefs.pyemits nine constraints forBGP_NEIGHBOR_AF— theroute-maps, prefix-lists, AS-path sets and
vrf_name— and none for the leafthat decides whether the row refers to a real peer.
KEY_PREFIX_REFSis therefore hand-maintained, and lives beside the validatorlogic rather than in
_generated/, which is marked do-not-edit. Each entryrecords the YANG path it encodes.
Expressing the rule as a key prefix match is not a shortcut — the leafref
plus its
vrf_namepredicate is exactly "the first two key components mustmatch", so VRF scoping falls out rather than being bolted on.
Second commit
A regression test only, and worth saying why it is not a fix.
_iter_leafref_values()evaluates"|" not in row_keyfor single-key tables.A non-string row key makes that membership test raise
TypeError, sovalidate_config()propagates an exception instead of returning aValidationResult— the one thing a validator should not do, since the callercannot then distinguish a malformed config from a broken validator.
This branch originally carried a guard for it.
mainhas since grown a broaderone:
_iter_leafref_values()yields nothing for a non-string row key beforereaching the membership test, which covers every shape rather than that one
call. The guard is dropped here as redundant and the test kept, so the
behaviour stays pinned.
Unreachable for JSON input, whose object keys are always strings; reachable
through the in-memory dict the function also accepts.
Verification
Test-first; the tests failed in two different functions, which is what
surfaced the second bug.
Measured over every ConfigDB artifact available, with the check counting only
its own errors:
config_db.jsonfrom a live fleetEvery instance of the broken shape is caught, and nothing in 86 rows of
coherent config is. The goldens appear twice on purpose: the same check reads
them as entirely broken before the keying fix and entirely clean after, which
is the distinction it exists to draw. None of those artifacts are in this
branch — the goldens live on a draft series, the fleet configs are not public
— so the figures are not reproducible from it alone.
3249 passedintests/unit(3 pre-existing xfails), flake8 and black clean.Scope
Nothing consumes this automatically during a sync:
validate_config()isreached through
osism sonic validateand through the unit test that runs thevalidator over committed ConfigDB artifacts. No artifact in
maincarries aBGP_NEIGHBOR_AFrow today, so this changes no pipeline result — it makes thedefect reportable, and gives the keying fix something to be measured against:
🤖 Generated with Claude Code