From 86d059db737bd4fe20f39601296c95e24733d732 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Tue, 25 Aug 2026 16:02:54 +0200 Subject: [PATCH 1/2] feat(sonic): validate BGP_NEIGHBOR_AF key references 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 --- osism/tasks/conductor/sonic/validator.py | 75 ++++++++++++++++ .../tasks/conductor/sonic/test_validator.py | 86 +++++++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index 096e9958c..72dd3a635 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -24,6 +24,42 @@ ) +@dataclass(frozen=True) +class KeyPrefixRef: + """A reference carried by a composite row key rather than by a row field. + + ``source_table`` rows are keyed ``a|b|...``; the first ``prefix_len`` + components must together name an existing key in ``target_table``. This + expresses the YANG leafrefs whose path is relative and predicated, which + :mod:`osism.tasks.conductor.sonic._generated._leafrefs` cannot represent: + ``parse_leafref_path()`` returns ``None`` for both shapes, so the generator + emits no constraint for them. Hand-maintained for that reason. + """ + + source_table: str + target_table: str + prefix_len: int + yang_path: str + + +# BGP_NEIGHBOR_AF.neighbor is a leafref into BGP_NEIGHBOR restricted to the same +# VRF, so the vrf_name|neighbor prefix of an AF key must name a real neighbor. +# Without this, an AF row can activate an address family for a peer that has no +# BGP_NEIGHBOR entry, and the neighbor it does name is left with no address +# family at all. +KEY_PREFIX_REFS = ( + KeyPrefixRef( + source_table="BGP_NEIGHBOR_AF", + target_table="BGP_NEIGHBOR", + prefix_len=2, + yang_path=( + "../../../BGP_NEIGHBOR/BGP_NEIGHBOR_LIST" + "[vrf_name=current()/../vrf_name]/neighbor" + ), + ), +) + + @dataclass class ValidationError: message: str @@ -118,6 +154,7 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: leafref_errors, leafref_warnings = _check_leafrefs(config) errors.extend(leafref_errors) warnings.extend(leafref_warnings) + errors.extend(_check_key_prefix_refs(config)) return ValidationResult(valid=not errors, errors=errors, warnings=warnings) @@ -346,3 +383,41 @@ def _format_missing_message(constraint: LeafrefConstraint, value: str) -> str: f"leafref {constraint.source_field}={value!r} does not resolve to " f"an existing entry in {targets}" ) + + +def _check_key_prefix_refs(config: Dict[str, Any]) -> List[ValidationError]: + """Verify every composite-key reference in :data:`KEY_PREFIX_REFS` resolves. + + Rows whose key has too few components are skipped: key arity is the row + schema's business, and reporting it here as well would double up on one + defect. + """ + errors: List[ValidationError] = [] + for ref in KEY_PREFIX_REFS: + rows = config.get(ref.source_table) + if not isinstance(rows, dict): + continue + target_keys = config.get(ref.target_table) + target_keys = set(target_keys) if isinstance(target_keys, dict) else set() + for row_key in rows: + if not isinstance(row_key, str): + # Malformed row; the row schema reports it. Reporting here too + # would turn one defect into two. + continue + parts = row_key.split("|") + if len(parts) <= ref.prefix_len: + continue + prefix = "|".join(parts[: ref.prefix_len]) + if prefix not in target_keys: + errors.append( + ValidationError( + message=( + f"{ref.source_table} key {row_key!r} references " + f"{ref.target_table} entry {prefix!r}, which does " + f"not exist" + ), + path=row_key, + table=ref.source_table, + ) + ) + return errors diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 67cc103d4..02e4874e5 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -643,3 +643,89 @@ def test_multi_target_reference_is_judged_when_every_target_is_present(): assert any( e.table == "VLAN_MEMBER" and "Ethernet999" in e.message for e in errors ), errors + + +def _key_ref_errors(result): + return [e for e in result.errors if "which does not exist" in e.message] + + +def test_af_key_resolves_when_neighbor_keyed_the_same_way(): + config = { + "BGP_NEIGHBOR": {"default|192.0.2.1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": { + "default|192.0.2.1|ipv4_unicast": {"admin_status": "up"}, + }, + } + assert _key_ref_errors(validate_config(config)) == [] + + +def test_af_key_flagged_when_neighbor_keyed_by_address_but_af_by_interface(): + """The shape the generator emits on the physical and port-channel paths.""" + config = { + "BGP_NEIGHBOR": {"default|192.0.2.1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": { + "default|Ethernet0|ipv4_unicast": {"admin_status": "up"}, + }, + } + errors = _key_ref_errors(validate_config(config)) + assert len(errors) == 1 + assert "default|Ethernet0|ipv4_unicast" in errors[0].message + assert "default|Ethernet0" in errors[0].message + assert errors[0].table == "BGP_NEIGHBOR_AF" + + +def test_af_key_resolves_for_an_unnumbered_pair(): + config = { + "BGP_NEIGHBOR": {"default|PortChannel1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": { + "default|PortChannel1|ipv4_unicast": {"admin_status": "up"}, + "default|PortChannel1|l2vpn_evpn": {"admin_status": "up"}, + }, + } + assert _key_ref_errors(validate_config(config)) == [] + + +def test_af_key_is_scoped_to_its_vrf(): + """A neighbor of the same name in another VRF must not satisfy the reference.""" + config = { + "BGP_NEIGHBOR": {"default|192.0.2.1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": { + "Vrf1|192.0.2.1|ipv4_unicast": {"admin_status": "up"}, + }, + } + errors = _key_ref_errors(validate_config(config)) + assert len(errors) == 1 + assert "Vrf1|192.0.2.1" in errors[0].message + + +def test_af_key_flagged_when_neighbor_table_is_absent(): + config = { + "BGP_NEIGHBOR_AF": { + "default|192.0.2.1|ipv4_unicast": {"admin_status": "up"}, + }, + } + assert len(_key_ref_errors(validate_config(config))) == 1 + + +def test_af_key_with_too_few_components_is_left_alone(): + """Key arity is the row schema's business; this check must not double-report.""" + config = { + "BGP_NEIGHBOR": {"default|192.0.2.1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": {"default|192.0.2.1": {"admin_status": "up"}}, + } + assert _key_ref_errors(validate_config(config)) == [] + + +def test_non_string_row_key_is_reported_not_raised(): + """A validator must return a result for malformed input, never raise. + + JSON keys are always strings, but ``validate_config`` is public and also + takes in-memory dicts, where a non-string key is reachable. + """ + config = { + "BGP_NEIGHBOR": {"default|192.0.2.1": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": {7: {"admin_status": "up"}}, + } + result = validate_config(config) + assert not result.valid + assert _key_ref_errors(result) == [] From c864541d161287e6a44567408fcf4a8c51cfbffc Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Tue, 25 Aug 2026 16:10:53 +0200 Subject: [PATCH 2/2] test(sonic): cover non-string row keys _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 --- tests/unit/tasks/conductor/sonic/test_validator.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 02e4874e5..c841ac6a1 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -729,3 +729,9 @@ def test_non_string_row_key_is_reported_not_raised(): result = validate_config(config) assert not result.valid assert _key_ref_errors(result) == [] + + +def test_non_string_row_key_in_a_simple_key_table_is_reported_not_raised(): + config = {"INTERFACE": {7: {}}, "PORT": {"Ethernet0": {"lanes": "0"}}} + result = validate_config(config) + assert not result.valid