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..c841ac6a1 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -643,3 +643,95 @@ 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) == [] + + +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