bgpd: fix AS display inconsistency for auto-created BGP instance#21887
bgpd: fix AS display inconsistency for auto-created BGP instance#21887rvaideesh wants to merge 1 commit into
Conversation
When a BGP VRF instance is auto-created by EVPN L3VNI configuration before the explicit "router bgp" command is processed, only bgp->as is updated but bgp->as_pretty is not updated. This caused show running-config to display the old AS number while bgp protocol used the correct one, leading to inconsistency between frr.conf and show running-config. Note: The issue is observed when Default and non-default BGP instance has different ASN values 1. Zebra processes the following config where tenant vrf association to L3VNI (example "vrf X vni 1") 2. Zebra receivdes the SVI is operational up event which is associated to VRF X Zebra marks the VNI ready and sends L3VNI UP notification to BGPd 3. Upon receiving L3VNI up, BGPd does not have vrf instance, so it auto-creates BGP VRF X with default VRF's ASN value (asn1) 4. Subsequently when BGPd reads "router bgp <asn2> vrf X" updates the bgp vrf instance's only the bgp->as field and not bgp->as_pretty (a BUG) 5. bgp->as_pretty remains with default VRF instances' ASN value (<asn1>) So the running config has stale info for BGP vrf instances, this leads to an issue when a user tries to apply any config using frr-reload, it ends up restarting FRR as ASN values do not match for the BGP VRF instances. This leads to unexpected BGP sessions flap due to restart of FRR service. Fix: To update both fileds of tenant VRF BGP instance's bgp->as and bgp->as_pretty value when a user configured bgp vrf instance is created. (the trigger would be "router bgp vrfX asn2) Signed-off-by: Vaideesh Ravi Shankar <vaideeshr@nvidia.com>
Greptile SummaryThis PR fixes a stale-display bug where
Confidence Score: 4/5The change is targeted and fixes a real inconsistency between bgp->as and bgp->as_pretty for auto-created VRF instances; the risk surface is narrow. The as_pretty allocation logic correctly mirrors bgp_create, and the !CHECK_FLAG guard is safe because auto-created VRFs are always initialised with ASNOTATION_UNDEFINED. A minor inconsistency remains: when asnotation == ASNOTATION_UNDEFINED on entry, the new code does not fall back to asn_str2asn_notation to re-derive notation from the updated pretty-string, whereas bgp_create always does. bgpd/bgpd.c — specifically the asnotation handling block around line 4130. Important Files Changed
Sequence DiagramsequenceDiagram
participant Z as Zebra
participant B as BGPd
participant VRF as BGP VRF Instance
Z->>B: L3VNI UP (vrf X, l3vni 1)
Note over B: bgp_vrf not found
B->>VRF: "bgp_get_vty(as=asn1, vrf=X, ASNOTATION_UNDEFINED)"
Note over VRF: Auto-created with default VRF asn1<br/>as_pretty=asn1, BGP_VRF_AUTO set
B->>B: Read config router bgp asn2 vrf X
B->>B: "bgp_lookup_by_as_name_type(as=asn2, vrf=X)"
Note over B: bgp->as != asn2, BGP_VRF_AUTO set
B->>VRF: "bgp->as = asn2"
B->>VRF: XFREE + XSTRDUP as_pretty for asn2
B->>VRF: Conditionally update asnotation
B->>VRF: UNSET_FLAG(BGP_VRF_AUTO)
Note over VRF: as=asn2, as_pretty=asn2<br/>show running-config now consistent
Reviews (1): Last reviewed commit: "bgpd: fix AS display inconsistency for a..." | Re-trigger Greptile |
| if (!CHECK_FLAG(bgp->config, BGP_CONFIG_ASNOTATION) && | ||
| asnotation != ASNOTATION_UNDEFINED) { | ||
| bgp->asnotation = asnotation; | ||
| SET_FLAG(bgp->config, BGP_CONFIG_ASNOTATION); | ||
| } |
There was a problem hiding this comment.
asnotation fallback missing vs. bgp_create
In bgp_create (lines 3739–3743), when asnotation == ASNOTATION_UNDEFINED, the notation is re-derived from the new as_pretty via asn_str2asn_notation. This block skips that fallback, so when no explicit notation is passed and BGP_CONFIG_ASNOTATION is not yet set, bgp->asnotation retains the old value (derived from asn1) even though as_pretty now reflects asn2. Adding an else branch calling asn_str2asn_notation(bgp->as_pretty, NULL, &bgp->asnotation) would keep this path consistent with bgp_create.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bgpd/bgpd.c
Line: 4130-4134
Comment:
**`asnotation` fallback missing vs. `bgp_create`**
In `bgp_create` (lines 3739–3743), when `asnotation == ASNOTATION_UNDEFINED`, the notation is re-derived from the new `as_pretty` via `asn_str2asn_notation`. This block skips that fallback, so when no explicit notation is passed and `BGP_CONFIG_ASNOTATION` is not yet set, `bgp->asnotation` retains the old value (derived from asn1) even though `as_pretty` now reflects asn2. Adding an `else` branch calling `asn_str2asn_notation(bgp->as_pretty, NULL, &bgp->asnotation)` would keep this path consistent with `bgp_create`.
How can I resolve this? If you propose a fix, please make it concise.|
@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 |
🟠 Waiting for conditions to matchDetails
|
|
Comment from greptile seems legit, could you check it? |
When a BGP VRF instance is auto-created by EVPN L3VNI configuration before the explicit "router bgp" command is processed, only bgp->as is updated but bgp->as_pretty is not updated.
This caused show running-config to display the old AS number while bgp protocol used the correct one, leading to inconsistency between frr.conf and show running-config.
Note: The issue is observed when Default and non-default BGP instance has different ASN values
So the running config has stale info for BGP vrf instances, this leads to an issue when a user tries to apply any config using frr-reload, it ends up restarting FRR as ASN values do not match for the BGP VRF instances. This leads to unexpected BGP sessions flap due to restart of FRR service.
Fix: To update both fileds of tenant VRF BGP instance's bgp->as and bgp->as_pretty value when a user configured bgp vrf instance is created. (the trigger would be "router bgp vrfX asn2)