From 99eb7a8083f71075441d1844ac9b26ca154483eb Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Sun, 30 Aug 2026 15:07:47 +0200 Subject: [PATCH] fix(sonic): keep AF admin_status true/false The vendored community YANG types BGP_NEIGHBOR_AF.admin_status as the up/down enumeration shared with PORT, so the validator flags every generated address-family row. Reading that as a defect in the generator points at the wrong fix: emitting the model's spelling would produce a config the switches do not act on. The table belongs to the unified FRR management interface, and that feature's own ConfigDB schema types the leaf as a boolean, in the same document where PORT is up/down. Its consumer agrees: in frrcfgd the address-family admin_status entries of nbr_af_key_map are bound to the token pair true/false, and a value outside that pair yields no command at all -- the neighbor is never activated for the address family, and a config that reads correctly produces a peering that carries no routes. Upstream resolved the contradiction in the consumer rather than in the model, teaching frrcfgd up/down in addition to true/false so that existing deployments keep working; that change is in community master and 202505 but not in 202411 or older, which covers the builds in the field today. So the generator's true/false is right and the model is the wrong side here. Opting the whole table out via PLATFORM_DIVERGENT_TABLES would also give up the neighbor leafref and every other field, so add a field-granular list beside it: PLATFORM_DIVERGENT_FIELDS retypes one leaf to what the platform accepts and emits the reason as a comment on the generated field. The rest of the table stays validated, and so does the leaf -- against the platform rather than against the model, which makes re-introducing the model's spelling an error rather than a silent regression. The documentation records what would let the entry be dropped again -- switches running builds whose frrcfgd carries the upstream change, established on a device rather than from a version number, since no published mapping ties an Enterprise build to its community branch -- and that both sides have to move together. DocImpact Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- Giltfile.yaml | 5 +- docs/sonic-config-validation.md | 64 +++++++++++++++++++ .../conductor/sonic/_generated/_schemas.py | 5 +- .../tasks/conductor/sonic/test_validator.py | 49 ++++++++++++++ tools/sonic_yang_to_pydantic.py | 43 ++++++++++++- 5 files changed, 160 insertions(+), 6 deletions(-) diff --git a/Giltfile.yaml b/Giltfile.yaml index 66f2a59ec..863984dfe 100644 --- a/Giltfile.yaml +++ b/Giltfile.yaml @@ -23,8 +23,9 @@ # # Note also that the models are community SONiC while the supported HWSKUs run # Enterprise SONiC builds. Where the two disagree the table is opted out via -# PLATFORM_DIVERGENT_TABLES in tools/sonic_yang_to_pydantic.py; re-check that -# list when this pin moves. +# PLATFORM_DIVERGENT_TABLES in tools/sonic_yang_to_pydantic.py, or a single leaf +# is retyped via PLATFORM_DIVERGENT_FIELDS beside it; re-check both lists when +# this pin moves. giltDir: ~/.gilt/clone debug: false parallel: true diff --git a/docs/sonic-config-validation.md b/docs/sonic-config-validation.md index 1e27f196b..fa0236dd9 100644 --- a/docs/sonic-config-validation.md +++ b/docs/sonic-config-validation.md @@ -46,6 +46,70 @@ Tables like these are listed in `PLATFORM_DIVERGENT_TABLES` in the generator. They get no schema and are reported as a warning naming the reason, instead of producing errors about values that are correct. +Where the divergence is one leaf rather than a whole table, dropping the table +gives up too much. Those are listed in `PLATFORM_DIVERGENT_FIELDS` instead, +which retypes just that leaf to what the platform accepts and emits the reason +as a comment beside the generated field. The rest of the table stays validated, +and so does the leaf — against the platform rather than against the model. + +| field | community model | what the devices run | +|--------------------------------|-----------------|----------------------| +| `BGP_NEIGHBOR_AF.admin_status` | enum `up`/`down`| `true`/`false` | + +### Why `BGP_NEIGHBOR_AF.admin_status` is `true`/`false` + +This one is worth spelling out, because the community model is not merely a +different flavour here — following it takes BGP down on the builds we run. + +`BGP_NEIGHBOR_AF` belongs to the unified FRR management interface, and that +feature's own CONFIG_DB schema types the leaf as a boolean, in the same +document where `PORT` and the interface tables are `up`/`down`: + + admin_status = "true" / "false" ; Neighbor admin status + +— `doc/mgmt/SONiC_Design_Doc_Unified_FRR_Mgmt_Interface.md` §3.2.1.7 in +`sonic-net/SONiC`. + +The consumer agrees. In `frrcfgd`, the address-family `admin_status` entries of +`nbr_af_key_map` are bound to the token pair `['true', 'false', False]`. A +value outside that pair does not fall back to anything: `get_command_cmn` logs +`Input token up is neither true or false` and returns no command, so +`neighbor activate` is never issued and the address family is silently left +inactive. A config that reads correctly produces a peering that carries no +routes. + +The community YANG model says otherwise — `stypes:admin_status`, the `up`/`down` +enumeration — and upstream resolved the contradiction in the consumer, not in +the model: `frrcfgd` was taught to accept `up`/`down` *in addition to* +`true`/`false`, explicitly so that existing deployments keep working +([sonic-buildimage#21697][af-admin], merged 2025-03-12). That change is in +community `master` and `202505`; it is **not** in `202411`, `202405` or +`202311`. + +So the generator emits `true`, and this entry keeps the validator from calling +that an error. + +### When this entry can go + +When every switch OSISM manages runs a build whose `frrcfgd` carries that +change. Then `up`/`down` becomes the better value — it satisfies both the model +and the consumer, and `config reload` on newer `sonic-utilities` YANG-validates +`/etc/sonic/config_db.json` and aborts on a value outside the model. + +Establish it on a device, not from a release note. Enterprise builds are cut +from community branches, but which branch a given build carries is not +something a version number answers, and no published mapping settles it. The +check is in the `bgp` container's `frrcfgd.py`: the `admin_status|ipv4` entry +of `nbr_af_key_map` must name a handler function rather than the literal token +list. + +Change both sides together. Emitting `up` while this entry still says +`true`/`false` turns every generated AF row into a validation error; retyping +the leaf while the generator still emits `true` does the same in reverse. And +neither is safe until the fleet has moved, whatever the validator says. + +[af-admin]: https://github.com/sonic-net/sonic-buildimage/pull/21697 + Vendoring the devices' own models instead is not currently possible: there is no authoritative published Enterprise model set. The management-framework lineage in `sonic-net/sonic-mgmt-common` carries only a handful of modules, the diff --git a/osism/tasks/conductor/sonic/_generated/_schemas.py b/osism/tasks/conductor/sonic/_generated/_schemas.py index 6d2cc8f71..fef87335b 100644 --- a/osism/tasks/conductor/sonic/_generated/_schemas.py +++ b/osism/tasks/conductor/sonic/_generated/_schemas.py @@ -663,7 +663,10 @@ class BgpNeighborAfListRow(BaseModel): vrf_name: Optional[str] = None neighbor: Optional[str] = None afi_safi: Optional[str] = None - admin_status: Optional[Literal["up", "down"]] = None + # Platform divergence: the frr-mgmt-framework CONFIG_DB schema types this + # leaf true/false, and frrcfgd only activates the address family for those + # two tokens; see docs/sonic-config-validation.md + admin_status: Optional[Literal["true", "false"]] = None send_default_route: Optional[bool] = None default_rmap: Optional[str] = None max_prefix_limit: Optional[Annotated[int, Field(ge=0, le=4294967295)]] = None diff --git a/tests/unit/tasks/conductor/sonic/test_validator.py b/tests/unit/tasks/conductor/sonic/test_validator.py index 67cc103d4..60fbfa831 100644 --- a/tests/unit/tasks/conductor/sonic/test_validator.py +++ b/tests/unit/tasks/conductor/sonic/test_validator.py @@ -393,6 +393,17 @@ def _warnings_for(result, table): return [w for w in result.warnings if table in w] +def _bgp_neighbor_af_config(admin_status): + """A BGP_NEIGHBOR_AF row with the neighbor its key references, so nothing + but the admin_status value is under test.""" + return { + "BGP_NEIGHBOR": {"default|Ethernet0": {"peer_type": "external"}}, + "BGP_NEIGHBOR_AF": { + "default|Ethernet0|ipv4_unicast": {"admin_status": admin_status}, + }, + } + + def test_platform_divergent_table_is_not_schema_validated(): """The vendored models are community SONiC; these devices run an Enterprise build that models SYSLOG_SERVER with different field names and @@ -430,6 +441,44 @@ def test_platform_divergent_mgmt_port_accepts_the_device_value(): assert [e for e in result.errors if e.table == "MGMT_PORT"] == [], result.errors +def test_platform_divergent_af_admin_status_accepts_the_device_value(): + """BGP_NEIGHBOR_AF.admin_status is true/false on the target platform: that + is what the frr-mgmt-framework CONFIG_DB schema specifies and the only + spelling frrcfgd turns into `neighbor activate`. The community model + types the leaf as the up/down enum shared with PORT.""" + config = _bgp_neighbor_af_config("true") + result = validate_config(config) + assert [ + e for e in result.errors if e.table == "BGP_NEIGHBOR_AF" + ] == [], result.errors + + +def test_platform_divergent_af_admin_status_rejects_the_yang_spelling(): + """`up` is what the community YANG model asks for and what the platform + ignores: frrcfgd never activates the address family for it. The carve-out + keeps the field validated against the platform rather than dropping it, + so emitting the model's spelling here is an error.""" + result = validate_config(_bgp_neighbor_af_config("up")) + assert [ + e for e in result.errors if e.table == "BGP_NEIGHBOR_AF" + ] != [], result.errors + + +def test_platform_divergent_field_leaves_the_rest_of_the_table_validated(): + """Overriding one leaf must not cost the table its schema the way + PLATFORM_DIVERGENT_TABLES does — its other fields stay checked.""" + config = _bgp_neighbor_af_config("true") + config["BGP_NEIGHBOR_AF"]["default|Ethernet0|ipv4_unicast"][ + "send_community" + ] = "foobar" + result = validate_config(config) + assert any( + "send_community" in e.path + for e in result.errors + if e.table == "BGP_NEIGHBOR_AF" + ), result.errors + + def test_platform_divergent_table_drops_its_own_leafrefs(): """SYSLOG_SERVER.vrf is a community-only field — the platform spells it vrf_name — so the constraint sourced from it must go with the schema.""" diff --git a/tools/sonic_yang_to_pydantic.py b/tools/sonic_yang_to_pydantic.py index e132b19f2..21cc54cee 100644 --- a/tools/sonic_yang_to_pydantic.py +++ b/tools/sonic_yang_to_pydantic.py @@ -22,6 +22,7 @@ import re import subprocess import sys +import textwrap from dataclasses import dataclass from pathlib import Path from typing import Dict, List, Optional, Tuple @@ -82,6 +83,23 @@ "MGMT_PORT": ("the platform models autoneg as a boolean, not as `on`/`off`"), } +# Individual leaves the vendored models type differently from the platform, +# where opting the whole table out via PLATFORM_DIVERGENT_TABLES would give up +# too much. Maps (table, leaf) to the annotation the platform actually accepts +# plus the reason, which is emitted as a comment beside the generated field. +# +# Same evidentiary bar as PLATFORM_DIVERGENT_TABLES, and the same trap: our own +# generated configs are not evidence. Each entry needs the platform's own +# schema or its consumer, and a note on what would let it be dropped again. +PLATFORM_DIVERGENT_FIELDS = { + ("BGP_NEIGHBOR_AF", "admin_status"): ( + 'Optional[Literal["true", "false"]] = None', + "the frr-mgmt-framework CONFIG_DB schema types this leaf true/false, " + "and frrcfgd only activates the address family for those two tokens; " + "see docs/sonic-config-validation.md", + ), +} + YANG_INT_BOUNDS = { "int8": (-(2**7), 2**7 - 1), "int16": (-(2**15), 2**15 - 1), @@ -577,13 +595,32 @@ def default_for_type(default_arg: str, annotation: str) -> str: return repr(default_arg) -def leaf_field_decl(leaf_stmt) -> str: +def leaf_field_decl(leaf_stmt, table_name: Optional[str] = None) -> str: + field_name, alias = safe_field_name(leaf_stmt.arg) + + override = ( + PLATFORM_DIVERGENT_FIELDS.get((table_name, leaf_stmt.arg)) + if table_name is not None + else None + ) + if override is not None: + annotation, reason = override + if alias: + raise NotImplementedError( + f"platform-divergent field {table_name}.{leaf_stmt.arg} needs an " + "alias, which the override does not carry" + ) + comment = "\n".join( + f" # {line}" + for line in textwrap.wrap(f"Platform divergence: {reason}", width=74) + ) + return f"{comment}\n {field_name}: {annotation}" + py = ( yang_type_to_py(leaf_stmt.search_one("type")) if leaf_stmt.search_one("type") else PyType("Any") ) - field_name, alias = safe_field_name(leaf_stmt.arg) mandatory = is_mandatory(leaf_stmt) default_stmt = leaf_stmt.search_one("default") @@ -682,7 +719,7 @@ def generate_row_class( rows = [] for leaf in leaves: if leaf.keyword == "leaf": - rows.append(leaf_field_decl(leaf)) + rows.append(leaf_field_decl(leaf, table_name)) elif leaf.keyword == "leaf-list": rows.append(leaf_list_field_decl(leaf, table_name)) if not rows: