From 48f963c499ab5c43132c31753a5eccba82221293 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 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 --- 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 1b96d10b5..bbc710ac8 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -20,6 +20,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 @@ -85,6 +121,7 @@ def validate_config(config: Dict[str, Any]) -> ValidationResult: ) errors.extend(_check_leafrefs(config)) + errors.extend(_check_key_prefix_refs(config)) return ValidationResult(valid=not errors, errors=errors, warnings=warnings) @@ -184,3 +221,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 3de928d34..c9389eeab 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -176,3 +176,89 @@ def test_unknown_table_emits_warning_not_error(): result = validate_config(config) assert any("NOT_A_REAL_TABLE" in w for w in result.warnings) assert _leafref_errors(result) == [] + + +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 f9da38dedeb3fd13ff422f855d7ad4d4d2a1575d Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Tue, 25 Aug 2026 16:10:53 +0200 Subject: [PATCH 2/2] fix(sonic): keep leafref check from raising on odd keys _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 --- osism/tasks/conductor/sonic/validator.py | 6 +++++- tests/unit/tasks/conductor/sonic/test_validator.py | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index bbc710ac8..bc5dd2b76 100644 --- a/osism/tasks/conductor/sonic/validator.py +++ b/osism/tasks/conductor/sonic/validator.py @@ -193,7 +193,11 @@ def _iter_leafref_values( raw: Any = None if isinstance(row, dict) and constraint.source_field in row: raw = row[constraint.source_field] - elif constraint.source_is_simple_key and "|" not in row_key: + elif ( + constraint.source_is_simple_key + and isinstance(row_key, str) + and "|" not in row_key + ): # Single-key list: row key directly carries the leaf value. raw = row_key diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index c9389eeab..65ef1a23d 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -262,3 +262,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