[bgp] Render BGP_AGGREGATE_ADDRESS into classic bootstrap bgpd.conf to avoid transient leak on bgp restart#28570
Conversation
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…o avoid transient leak on bgp restart Signed-off-by: Guangyao Zhao <guangyaozhao@microsoft.com>
eae17c9 to
7696c19
Compare
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates the classic-path FRR bootstrap config generation so BGP_AGGREGATE_ADDRESS (and its associated prefix-lists) are rendered into the initial bgpd.conf, eliminating a restart/boot window where contributing more-specifics could be transiently advertised before bgpcfgd reconciliation applies suppression.
Changes:
- Include a new aggregate-address bootstrap fragment when
BGP_AGGREGATE_ADDRESSis present. - Add
bgpd.aggregate.conf.j2to render aggregate-address and related prefix-lists early (with strict prefix + BBR gating).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dockers/docker-fpm-frr/frr/bgpd/bgpd.conf.j2 | Conditionally includes the new aggregate bootstrap fragment before the main bgpd config. |
| dockers/docker-fpm-frr/frr/bgpd/bgpd.aggregate.conf.j2 | New template that renders BGP_AGGREGATE_ADDRESS and prefix-lists into bootstrap bgpd.conf. |
| {% if BGP_AGGREGATE_ADDRESS is defined %} | ||
| {% include "bgpd.aggregate.conf.j2" %} | ||
| ! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
dockers/docker-fpm-frr/frr/bgpd/bgpd.aggregate.conf.j2:28
- The “Idempotency” comment claims the command strings must mirror managers_aggregate_address.py “byte-for-byte”, but the template intentionally emits prefix-list entries with an explicit
seqwhile the manager emits noseq(so these lines cannot be byte-for-byte identical). This is misleading documentation and makes future maintenance riskier.
! Idempotency:
! * command strings MUST mirror managers_aggregate_address.py byte-for-byte:
! generate_aggregate_address_commands() and generate_prefix_list_commands().
! A later manager reconcile is then a FRR no-op (duplicate prefix-list value
! + identical aggregate-address are both dropped by FRR). If you edit one
! side, edit the other.
! * prefix-list entries use explicit seq >= 10 to avoid colliding with any
! existing lower-numbered prefix-list entries. The manager emits NO seq (FRR
! auto-assigns), and FRR dedups by value, so the differing seq does not matter.
| {% set bbr_on = (BGP_BBR is defined and 'all' in BGP_BBR and BGP_BBR['all'].get('status', 'disabled') == 'enabled') %} | ||
| {% if BGP_AGGREGATE_ADDRESS is defined and (DEVICE_METADATA is defined) and ('localhost' in DEVICE_METADATA) and ('bgp_asn' in DEVICE_METADATA['localhost']) and (DEVICE_METADATA['localhost']['bgp_asn'].lower() not in ['none', 'null']) %} | ||
| {% set bgp_asn = DEVICE_METADATA['localhost']['bgp_asn'] %} | ||
| {# Precompute whether any entry qualifies so we never emit empty stanzas. #} | ||
| {% set ns = namespace(any=false) %} | ||
| {% for prefix, attr in BGP_AGGREGATE_ADDRESS.items() %} | ||
| {% if ('/' in prefix) and ((prefix | ipv4) or (prefix | ipv6)) and ((prefix | ip | string | lower) == (prefix | ip_network | string | lower)) and (attr.get('bbr-required', 'false') != 'true' or bbr_on) %} | ||
| {% set ns.any = true %} |
|
This PR has backport request for branch(es): 202511,202512,202605,202608. ---Powered by SONiC BuildBot
|
|
This PR has backport request label(s) for branch(es): 202511,202605,msft-202512,msft-202608, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202511: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
On a T1 aggregation router, BGP_AGGREGATE_ADDRESS rows are realized into FRR only at runtime by bgpcfgds AggregateAddressMgr. At boot the peer managers bring T2 uplink sessions to Established concurrently with the aggregate manager installing suppression. The stock bgpd.conf carries no aggregate, so any T2 session reaching Established before the manager finishes advertises the contributing more-specifics to T2 unsuppressed for the duration of the window. This is a transient correctness/blast-radius leak at every cold boot / restart bgp.
Why I did it
Bootstrap race on cold boot /
restart bgp: contributing routes escape to T2 between session bring-up andAggregateAddressMgrreconciliation. Fix (template-only, no bgpcfgd code change): Render the aggregate-address plus the aggregate/contributing prefix-lists directly into the bootstrap bgpd.conf so suppression exists in FRR before bgpd forms any session.Work item tracking
How I did it
New
dockers/docker-fpm-frr/frr/bgpd/bgpd.aggregate.conf.j2: rendersBGP_AGGREGATE_ADDRESSinto bootstrapbgpd.confwith the following safety gates:validate_prefix(strict=True))bbr-required=trueentries only rendered whenBGP_BBR|all status == "enabled"is explicitly present in CONFIG_DB (deliberately ignoresconstants.ymldefault_state — cannot verify peer-group existence at bootstrap time)managers_aggregate_address.pybyte-for-byte so a later manager reconcile is a FRR no-opseq >= 10to avoid collision withpolicy.conf.j2anchor atseq 5Updated
dockers/docker-fpm-frr/frr/bgpd/bgpd.conf.j2: include the new template beforebgpd.main.conf.j2, guarded so it is only active whenBGP_AGGREGATE_ADDRESSis defined:How to verify it
BGP_AGGREGATE_ADDRESSentries in CONFIG_DB.systemctl restart bgp)./etc/frr/bgpd.conf— aggregate-address and prefix-list entries must be present before any session reaches Established.bgpcfgdreconciles, verifyAggregateAddressMgrreconcile is a FRR no-op (no config churn invtysh).Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
SONiC Software Version: SONiC.copilot_add-bgpd-aggregate-conf-template.0-3d1b6c161
SONiC OS Version: 13
Description for the changelog
Render BGP_AGGREGATE_ADDRESS into the classic-path bootstrap bgpd.conf to eliminate transient route-leak window on BGP restart.
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)