diff --git a/osism/tasks/conductor/sonic/validator.py b/osism/tasks/conductor/sonic/validator.py index 1b96d10b5..bc5dd2b76 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) @@ -156,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 @@ -184,3 +225,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..65ef1a23d 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -176,3 +176,95 @@ 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) == [] + + +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