feat(sonic): validate BGP_NEIGHBOR_AF key references - #2619
Open
ideaship wants to merge 2 commits into
Open
Conversation
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 this. Its 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 both shapes, so the generator emits no constraint. The referring
value also lives inside a composite row key rather than a row field,
which the generated checker documents as out of scope. 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.
Checked against the committed SONiC E2E goldens and two configs taken
from a live fleet: all 8 mismatched address-family rows in the goldens
are flagged, and none of the 74 rows in the fleet configs are, so the
check separates the two shapes it is meant to distinguish.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>
_iter_leafref_values() tests "|" 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. JSON object keys are always strings, so this 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 that produces non-string keys. Guard only the membership test, so rows that do carry the field are unaffected. The malformed key itself is left to the row schema, which already reports it. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
Stacked on #2618.
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 checked 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 cannot express it
_generated/_leafrefs.pyis produced bytools/sonic_yang_to_pydantic.py, whoseparse_leafref_path()returnsNonefor relative paths and for any path containing an XPath predicate. Thisone is both, so no constraint is emitted. The referring value also lives inside
a composite row key rather than a row field, which
_check_leafrefs()documents as deliberately out of scope.
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
_iter_leafref_values()evaluates"|" not in row_keyfor single-key tables. Anon-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. Pre-existing
and unreachable for JSON input, whose keys are always strings; reachable through
the in-memory dict the function also accepts. Split into its own commit because
it changes the behaviour of existing code.
Verification
Test-first; the tests failed in two different functions, which is what surfaced
the second bug.
Measured against the committed SONiC E2E goldens and two
config_db.jsontakenfrom a live fleet:
osism_e2e-spine-1goldenosism_e2e-leaf-1,osism_e2e-leaf-2goldensEvery instance of the shape known to be broken is caught; nothing in 74 rows of
real coherent config is.
3147 passedintests/unit(4 pre-existing xfails),flake8 and black clean.
Scope
Nothing consumes this automatically:
validate_config()is reached only throughosism sonic validate, never during a sync and not from any test or CI job. Sothis changes no pipeline result — it makes the defect reportable, and gives the
keying fix something to be measured against when it comes.
🤖 Generated with Claude Code