sonic-yang: model BGP afi_safi as an enumeration instead of an unconstrained string#28606
Open
bhouse-nexthop wants to merge 2 commits into
Open
sonic-yang: model BGP afi_safi as an enumeration instead of an unconstrained string#28606bhouse-nexthop wants to merge 2 commits into
bhouse-nexthop wants to merge 2 commits into
Conversation
…trained string
Add a bgp_afi_safi enumeration typedef in sonic-bgp-common (with a
bgp_afi_safi_global restriction that excludes the SR-Policy families) and
use it for the afi_safi list keys that were previously an unconstrained
'type string':
* sonic-bgp-cmn-af (BGP_NEIGHBOR_AF / BGP_PEER_GROUP_AF) uses the full
bgp_afi_safi vocabulary, including the SR-Policy families.
* BGP_GLOBALS_AF_LIST, BGP_GLOBALS_AF_AGGREGATE_ADDR_LIST and
BGP_GLOBALS_AF_NETWORK_LIST use bgp_afi_safi_global, so the SR-Policy
families are rejected at validation time.
Constraining the key rejects non-canonical spellings such as
'ipv4-unicast' at commit time and lets YANG-derived CLIs offer the valid
values as completion candidates. This is a model-only change; validation
and completion are both derived from the model.
Also tighten two neighbouring free-string leaves:
* rr_cluster_id (BGP_GLOBALS) becomes a union of an IPv4 address and a
non-zero 32-bit integer.
* the BGP confederation peers leaf gains a pattern enforcing its
semi-colon-separated sub-ASN shape.
Signed-off-by: Brad House - Nexthop <bhouse@nexthop.ai>
bhouse-nexthop
requested review from
dgsudharsan,
praveen-li,
qiluo-msft and
venkatmahalingam
as code owners
July 23, 2026 21:20
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
The
afi_safilist keys across the BGP models were declaredtype stringwith no enumeration, so any value was valid YANG. Bothipv4-unicast(hyphen) andipv4_unicast(underscore) validated and committed, producing two distinct AF rows — only the underscore spelling is functional, since CONFIG_DB, bgpcfgd and frrcfgd all use underscores.This is a model gap, not a CLI defect. nhcli derives all config-mode validation and completion from the YANG, so an unconstrained key is faithfully accepted; the fix belongs in the model. It also explains the original report (Randy Williams) that tab-completion walks you to the
afi-safiposition and then leaves you to type the value blind — there were no enum values for the CLI to offer.Two further BGP leaves had the same shape of problem — declared
type stringwhile their consumers accept only a specific form — and are fixed here too:BGP_GLOBALS.rr_cluster_idand theBGP_DEVICE_GLOBALconfederationpeerslist.How I did it
Added a
bgp_afi_safitypedef tosonic-bgp-common.yangwith the canonical underscore values, plus two restrictions for the contexts whose vocabulary is narrower, and wired all fiveafi_safileaves to them:BGP_NEIGHBOR_AF/BGP_PEER_GROUP_AFbgp_afi_safiipv4_unicast,ipv6_unicast,l2vpn_evpn,ipv4_srpolicy,ipv6_srpolicyBGP_GLOBALS_AFbgp_afi_safi_globalBGP_GLOBALS_AF_AGGREGATE_ADDRbgp_afi_safi_globalBGP_GLOBALS_AF_NETWORKbgp_afi_safi_globalBGP_DEVICE_GLOBAL_AFbgp_afi_safi_device_globalipv4_unicast,ipv6_unicastThe per-context sets are derived from the actual consumers, not from prose. SR-Policy is excluded from the globals tables because frrcfgd installs no global SR-Policy config (
frrcfgd.py:3618).BGP_DEVICE_GLOBAL_AFis unicast-only becauseAFI_SAFI_MAPinbgpcfgd/managers_device_global_af.py:9has exactly two entries andlog_warns away anything else. This follows how these files already model sibling leaves (bgp_peer_type,bgp_community_type,bgp_tx_add_paths_typeare all enumerations).Constraining the key type makes several hand-written guards unreachable, so they are removed:
must "not(contains(., 'srpolicy'))"clauses on theBGP_GLOBALS_AFlistsinstall_backup_pathunicastmustonBGP_DEVICE_GLOBAL_AF, whose key type now admits only the two unicast familiesThe equivalent
install_backup_pathguard onBGP_GLOBALS_AFis kept —l2vpn_evpnis still a valid key there, so that constraint is still live.*_multicastis deliberately excluded: no consumer implements it. TheBGP_DEVICE_GLOBAL_AF_LISTdescription previously advertisedipv4_multicast/ipv6_multicast/l2vpn_evpn, none of whichAFI_SAFI_MAPsupports; that description is corrected.No CLI code changes. libyang validates a list key against its declared type, so an out-of-enum
afi_safiis rejected at commit across every entry point, and the enum values become completion candidates.Additionally constrained (same class of gap, not
afi_safi):sonic-bgp-global.yangrr_cluster_id— renders intobgp cluster-id, which takes an IPv4 address or a 32-bit integer. Now a union ofinet:ipv4-addressanduint32 { range "1..4294967295"; }.sonic-bgp-device-global.yangCONFEDpeers— documented as semi-colon-separated sub-ASNs, previously unvalidated. Nowpattern "[1-9][0-9]*(;[1-9][0-9]*)*". The pattern enforces the shape only; it does not range-check each sub-ASN against the 32-bit AS maximum, which would need a leaf-list.The existing
"66000;63000"sample in the tests and indoc/Configuration.mdsatisfies the new pattern. No existingrr_cluster_idvalues exist anywhere in the repo.How to verify it
Build and see yang model tests succeed.
Which release branch to port
Tested branch (Please provide the tested image version)
Description for the changelog
sonic-yang: constrain BGP afi_safi list keys, rr_cluster_id and confederation peers instead of leaving them unconstrained strings
Link to config_db schema for YANG module changes
https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md#bgp_globals_af
A picture of a cute animal (not mandatory but encouraged)