fix(sonic): drop the AF key reference check - #2703
Conversation
![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 reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. Removing this check allows BGP_NEIGHBOR_AF entries whose VRF and neighbor prefix has no corresponding BGP_NEIGHBOR entry to pass validation and potentially be applied to a SONiC device. Reverting prevents future invalid configurations, but any accepted configuration or resulting routing impact would need to be corrected separately.
06ed26b to
b45f56a
Compare
KEY_PREFIX_REFS carried one entry: the BGP_NEIGHBOR_AF key prefix must name an existing BGP_NEIGHBOR row. The YANG leafref behind it is real (sonic-bgp-neighbor.yang:128), and the generator does emit the shape it flags -- BGP_NEIGHBOR keyed by the resolved peer address next to BGP_NEIGHBOR_AF keyed by the interface name. What makes the check untenable is that the change which would stop the generator emitting that shape was withdrawn. The two were written together and justified by the same leafref; keeping the detector while declining the fix leaves the tree in a state where it rejects output it also insists on producing. The first artifact to be committed that the generator produced would fail validation, and the only way to land it would be to weaken the artifact test or to bend fixtures around the check -- both worse than not having it. Exposure decided it. The flagged shape does not occur in the live fleet data examined: none of the 74 address-family rows in two config_db.json taken from a running fleet are affected, and the transfer-role prefixes the physical path needs to reach the shape at all are absent from the NetBox data that goes with them. The check fires only on synthetic data, so it costs a gate and buys nothing. The generic composite-key leafref support is a separate change and stays in place, so re-adding the entry is a few lines if a real occurrence ever turns up. Two tests are kept rather than reverted with the rest. Both pin that validate_config() returns a result instead of raising on a non-string row key, which holds regardless of this check; a later change built on the first one deliberately. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
b45f56a to
febfc9f
Compare
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. The validator will now accept a BGP_NEIGHBOR_AF row whose VRF/neighbor prefix has no corresponding BGP_NEIGHBOR entry, allowing an invalid routing configuration to be deployed and potentially disrupting peer or route activation. Reverting restores the validation check, but any configuration already accepted or resulting outage would need separate correction.
3 participants
Removes the one entry in
KEY_PREFIX_REFS: the check that aBGP_NEIGHBOR_AFrow key'svrf_name|neighborprefix names an existingBGP_NEIGHBORrow.Why
The check and the generator change that would satisfy it were written together
and justified by the same YANG leafref. The generator change was withdrawn:
The check was not, so the tree now rejects a shape it also insists on
producing. Nothing in it had to notice, because no artifact the generator
produces is committed anywhere —
test_validator_artifacts.pyvalidates asingle file today, and its glob over
tests/e2e/golden/matches nothing.The SONiC E2E series is the first thing to commit generator output, and it
cannot: ten address-family rows across four goldens fail validation, all of
them the interface-vs-address keying the withdrawn change addressed. The ways
out without this revert are to weaken the artifact test or to shape fixtures
around the check, and both are worse than not having the check.
Exposure
The flagged shape has not been found in the field. Two
config_db.jsontakenfrom a running fleet have zero affected rows out of 74 address-family rows, and
the transfer-role prefixes the physical path needs to reach the shape at all are
absent from the NetBox data that goes with them. The check fires only on
synthetic data.
What stays
The generic composite-key leafref support is a separate change and is
untouched, so re-adding the entry is a few lines if a real occurrence turns up.
Two tests are kept rather than reverted with the rest: both pin that
validate_config()returns a result instead of raising on a non-string rowkey, which holds either way, and a later change built on the first one
deliberately.
The goldens in the series above pin the keying as the generator emits it, so
they are where the shape is recorded from here on.
🤖 Generated with Claude Code