[sonic-bgpcfgd]: Validate dynamic peer-range names by baijupn · Pull Request #29448 · sonic-net/sonic-buildimage

baijupn · 2026-09-09T23:26:54Z

Why I did it

Dynamic BGP peer-range names and table-key components are used as identifiers
in generated FRR configuration and must be single-line values. Reject CR/LF
before these inputs reach configuration generation, dependency queues, or
configured-peer state.

Work item tracking

Microsoft ADO (number only): 39463597

How I did it

Reject CR/LF in dynamic BGP_PEER_RANGE.name and the complete raw table key,
including both components of VRF/VNET-qualified keys.
Validate before dependency evaluation in handler(), retain defensive
set_handler() validation for direct calls and stale queued events, and
protect direct del_handler() calls. Permanently invalid SET input is
consumed rather than queued for retry.
Preserve FRR configuration, Directory data, peer cache, and STATE_DB when
rejecting invalid events.
Add a single-line YANG typedef for peer_range_name and name in both
peer-range lists, plus a must constraint for the qualified VRF/VNET name.
Preserve existing union leafrefs, key/name equality, and optionality without
changing global VRF/VNET naming policy.
Add runtime and schema regression coverage for LF, CR, CRLF, qualified keys,
cached updates, direct deletes, dependency readiness, stale replay, mixed
retry queues, and valid IPv4/IPv6 behavior.
Isolate test operation constants across the BGP and shared manager modules
to avoid mocked-versus-real SWSS dispatch mismatches in the full CI suite.
Temporary diagnostic instrumentation has been removed.

How to verify it

From src/sonic-bgpcfgd, with the component dependencies available:

python3 -m pytest -q -o addopts="" \
    tests/test_bgp.py tests/test_bgp_dynamic_validation.py \
    tests/test_bgp_options.py tests/test_bgp_unnumbered.py tests/test_bgpmon.py \
    tests/test_directory.py tests/test_db.py

From src/sonic-yang-models:

python3 -m pytest -q tests/yang_model_pytests/test_bgp_peerrange.py

Canonical component targets from the repository root:

BUILD_SKIP_TEST=n SONIC_DPKG_CACHE_METHOD=none \
SONIC_DPKG_CACHE_METHOD_OVERRIDE=none \
make -f Makefile.work BLDENV=trixie \
    target/python-wheels/trixie/sonic_yang_models-1.0-py3-none-any.whl \
    target/python-wheels/trixie/sonic_bgpcfgd-1.0-py3-none-any.whl

Preserve and move existing target wheels aside before running the canonical
targets to ensure fresh execution rather than an up-to-date result.

Which release branch to backport (provide reason below if selected)

Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO): 39463597

Failure type: other

Tested branch

Test result

Local validation of the conflict-resolved head 85e0679dd, completed
September 30, 2026:

Validation	Result
Fresh canonical Trixie bgpcfgd wheel suite	2,782 passed
Fresh canonical Trixie YANG-model wheel suite	995 passed
Dependent config-engine wheel suite	355 passed

Both component wheels were generated successfully. Packaged BGP runtime/tests
and both YANG/CVL peer-range schema copies match the local source. SWSS rebuilt
successfully through the normal dependency chain. The merge reconciles the
shared neighbor/sentinel validation now present in master with this PR's
dynamic peer-range validation, and removes superseded queue-behavior
assertions. These are local component results, not a claim that remote Azure
end-to-end validation has passed.

Description for the changelog

Reject line breaks in dynamic BGP peer-range names and qualified keys at runtime
and in YANG, with regression coverage for queued events, updates, and deletes.

Link to config_db schema for YANG module changes

Updated CONFIG_DB BGP_PEER_RANGE YANG schema

A picture of a cute animal (not mandatory but encouraged)

Not included.

Require dynamic BGP peer-range names to be single-line values before FRR template processing. Reject CR and LF values before add or update handling and leave configuration state unchanged for unsupported names. Add regression coverage for LF, CR, and CRLF values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c208e923-9777-44d4-aab6-4da92b7abb44 Signed-off-by: Baiju Parameswaran <baijup@microsoft.com>

azure-pipelines · 2026-09-09T23:27:03Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

mssonicbld · 2026-09-09T23:27:12Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-09T23:27:27Z

Azure Pipelines: Successfully started running 1 pipeline(s).

baijupn · 2026-09-10T14:08:03Z

/azpw retry

mssonicbld · 2026-09-10T14:08:05Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-09-10T14:08:08Z

Retrying failed(or canceled) stages in build 1216257:

✅Stage Test:

Job impacted-area-kvmtest-t2 by Elastictest: retried.

azure-pipelines · 2026-09-10T14:17:54Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

🟡 Changes recommended

The new validation currently returns False (which implies “retry later” and can cause requeueing), and the added test can yield false positives without resetting the mocked logger per iteration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens sonic-bgpcfgd dynamic BGP peer-range handling by validating that peer-range names are single-line strings, preventing malformed multiline identifiers from generating invalid FRR configuration.

Changes:

Add validation in the BGP peer manager to reject dynamic peer-range names containing CR/LF characters.
Add regression tests covering \n, \r, and \r\n in dynamic peer-range names.

File summaries

File	Description
src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py	Adds a validation check for CR/LF in dynamic peer-range `name` before processing updates.
src/sonic-bgpcfgd/tests/test_bgp.py	Adds a new unit test verifying multiline dynamic peer-range names are rejected without updating state.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Treat rejected multiline dynamic peer-range names as permanently handled so Manager does not queue them for retry. Reset the logger mock per test case, require exactly one error, and verify invalid updates are not added to the retry queue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c208e923-9777-44d4-aab6-4da92b7abb44 Signed-off-by: Baiju Parameswaran <baijup@microsoft.com>

mssonicbld · 2026-09-10T14:32:20Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-10T14:32:39Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🟡 Changes recommended

The new validation runs only in set_handler(), so invalid multiline names can still be queued when dependencies are unmet, and the added “not queued” test doesn’t currently cover that queued code path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Lite

Validate dynamic peer-range names before dependency handling so permanent validation failures are not added to the retry queue. Keep set_handler validation as a defensive backstop and cover invalid, valid, non-dynamic, and delete handler paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c208e923-9777-44d4-aab6-4da92b7abb44 Signed-off-by: Baiju Parameswaran <baijup@microsoft.com>

mssonicbld · 2026-09-10T14:42:24Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-10T14:42:40Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🟡 Changes recommended

The new validation only checks data["name"] and should also validate the dynamic peer-range key to prevent CR/LF from reaching vtysh/peer-group identifiers in other code paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

qiluo-msft

F065 fix is incomplete — see thread on managers_bgp.py#L208. data["name"] is validated but the peer-range key (nbr from split_key(key)) isn't, and it reaches the same vtysh push path via get_existing_ip_ranges(), the delete/shutdown templates, and no listen range's peer_group=nbr. Requesting the key be validated the same way before this closes the reported vuln.

Validate CR/LF characters across qualified dynamic peer-range keys before dependency checks and retain the defensive set-handler guard. Add regression coverage for set, delete, direct handler, and qualified VRF/VNET key paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Baiju Parameswaran <baijup@microsoft.com>

mssonicbld · 2026-09-14T07:14:26Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-14T07:14:39Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-09-18T16:04:45Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-18T16:04:52Z

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.

Copilot

🟢 Approval recommended

Validation and regression coverage are present; the remaining comment is a documentation nit.

Review details

Suppressed comments (1)

src/sonic-yang-models/yang-models/sonic-bgp-peerrange.yang:34

The PR description says “N/A - no YANG module changes,” but this patch adds a new revision and changes the BGP peer-range schema with a typedef and must constraint. Please update the description (including the schema link section) to reflect this YANG model change, or remove these schema edits if they were unintended.

    revision 2026-09-18 {
        description
            "Reject line breaks in dynamic peer-range names and qualified keys.";
    }

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Lite

baijupn · 2026-09-22T09:59:11Z

/azpw retry

mssonicbld · 2026-09-22T09:59:14Z

Retrying failed(or canceled) jobs...

mssonicbld · 2026-09-22T09:59:15Z

No Azure DevOps builds found for #29448.

Resolve the manager conflict by retaining upstream key parsing and normalization alongside the dynamic peer-range CR/LF guards before queueing, SET, and DEL. Add dispatch regression coverage for upstream invalid-key rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c208e923-9777-44d4-aab6-4da92b7abb44 Signed-off-by: Baiju Parameswaran <baijup@microsoft.com>

baijupn · 2026-09-22T10:33:45Z

/azpw ms_conflict

mssonicbld · 2026-09-22T10:33:46Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-22T10:34:02Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved issues block approval.

Review effort: Lite
Findings: None

…29435) Why I did it BGP neighbor names must remain single-line descriptions. This PR also extends the scope to BGP Sentinel names, which are used as peer-group identifiers. Runtime and YANG validation consistently reject CR/LF values without changing existing single-line behavior. Work item tracking Microsoft ADO (number only): 39463596 How I did it Validate general, internal, monitor, VOQ chassis, and sentinel names before SET dependency queueing and before add/update dispatch. Log and consume invalid events without FRR, in-memory Directory, peer-cache, or configured STATE_DB changes. Direct calls and queued replay are covered. Rejection does not remove the originating CONFIG_DB record. Restrict the shared sonic-bgp-cmn-neigh/name leaf to [^\n\r]*. Extended scope: sentinels. Apply the same CR/LF-only runtime guard and constrain both sentinel_name and name in sonic-bgp-sentinel.yang, retaining the existing key/name equality constraint. Reject sentinel keys containing CR/LF independently of the name field, before dependency queueing. Reject non-string sentinel keys safely and consume invalid direct calls and queued replay without side effects. Preserve upstream key validation and existing requiredness/empty-value behavior; do not introduce a broader sentinel naming policy. Add 63 sentinel runtime cases and nine sentinel YANG negative cases, including CR/LF/CRLF, new/existing peers, dependency readiness, stale replay, no side effects, and valid IPv4/IPv6 add/update/delete behavior. Add another 158 runtime cases for key-only rejection with clean/empty/absent names, malformed key types, and preservation of valid queued IPv4/IPv6 peers. Defensively reject non-None, non-string names before CR/LF checks, with 280 regression cases across the five in-scope peer types and event paths. Normal SWSS inputs are strings; this hardens malformed direct Python calls without changing missing-name compatibility or dynamic handling. Dynamic-name scope and companion PR Dynamic peer-range work remains in #29448, tracked by ADO 39463597, and is not included in this PR. At companion head d52a0a3, runtime guards reject multiline dynamic keys/names before dependency queueing and SET dispatch. That is runtime coverage, not complete runtime-and-YANG coverage. Remaining companion work is to constrain dynamic peer_range_name and name in both VRF and template list variants of sonic-bgp-peerrange.yang, preserve equality constraints, add schema and explicit existing-peer update/replay regressions, and reconcile its shared handler changes and general-peer queue test with this PR. The dynamic/sentinel review discussion remains open.

qiluo-msft · 2026-09-29T20:30:05Z

Please resolve conflict

Merge current upstream master and reconcile the shared BGP peer-name validation with dynamic peer-range key and name checks. Remove superseded queue-behavior assertions now covered by the merged neighbor and sentinel validation. Signed-off-by: Baiju Parameswaran <baijup@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 99ed51af-eaba-47df-9d95-e565b1e72c3b

mssonicbld · 2026-09-30T01:02:26Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2026-09-30T01:02:42Z

Azure Pipelines: Successfully started running 1 pipeline(s).

baijupn · 2026-09-30T01:02:54Z

/azpw ms_conflict

baijupn · 2026-09-30T01:03:28Z

@qiluo-msft The conflict with current master is resolved in 85e0679dd, preserving both the merged neighbor/sentinel validation and this PR's dynamic peer-range checks. Fresh canonical Trixie validation passed (2,782 bgpcfgd, 995 YANG-model, and 355 config-engine tests). Could you please re-review the updated head?

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

baijupn marked this pull request as ready for review September 10, 2026 14:17

baijupn requested a review from StormLiangMS as a code owner September 10, 2026 14:17

Copilot AI lite review requested due to automatic review settings September 10, 2026 14:17

Copilot started reviewing on behalf of baijupn September 10, 2026 14:18 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py Outdated

Comment thread src/sonic-bgpcfgd/tests/test_bgp.py

Copilot AI review requested due to automatic review settings September 10, 2026 14:32

Copilot started reviewing on behalf of baijupn September 10, 2026 14:32 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py Outdated

Comment thread src/sonic-bgpcfgd/tests/test_bgp.py Outdated

Copilot AI review requested due to automatic review settings September 10, 2026 14:42

Copilot started reviewing on behalf of baijupn September 10, 2026 14:42 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py Outdated

qiluo-msft requested changes Sep 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 14, 2026 07:14

baijupn requested a review from venkatmahalingam as a code owner September 18, 2026 16:04

Copilot started reviewing on behalf of baijupn September 18, 2026 16:04 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 22, 2026 10:33

Copilot started reviewing on behalf of baijupn September 22, 2026 10:33 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

qiluo-msft previously approved these changes Sep 29, 2026

View reviewed changes

baijupn dismissed qiluo-msft’s stale review via 85e0679 September 30, 2026 01:02

Copilot AI lite review requested due to automatic review settings September 30, 2026 01:02

Copilot started reviewing on behalf of baijupn September 30, 2026 01:02 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

deepak-singhal0408 approved these changes Oct 1, 2026

View reviewed changes

qiluo-msft approved these changes Oct 1, 2026

View reviewed changes

qiluo-msft merged commit 69f88ba into sonic-net:master Oct 1, 2026
32 checks passed

Conversation

baijupn commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

Work item tracking

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Tested branch

Test result

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

azure-pipelines Bot commented Sep 9, 2026

Uh oh!

mssonicbld commented Sep 9, 2026

Uh oh!

azure-pipelines Bot commented Sep 9, 2026

Uh oh!

baijupn commented Sep 10, 2026

Uh oh!

mssonicbld commented Sep 10, 2026

Uh oh!

mssonicbld commented Sep 10, 2026

Uh oh!

azure-pipelines Bot commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Sep 10, 2026

Uh oh!

azure-pipelines Bot commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Sep 10, 2026

Uh oh!

azure-pipelines Bot commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Sep 14, 2026

Uh oh!

azure-pipelines Bot commented Sep 14, 2026

Uh oh!

mssonicbld commented Sep 18, 2026

Uh oh!

azure-pipelines Bot commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (1)

Uh oh!

baijupn commented Sep 22, 2026

Uh oh!

mssonicbld commented Sep 22, 2026

Uh oh!

mssonicbld commented Sep 22, 2026

Uh oh!

baijupn commented Sep 22, 2026

Uh oh!

mssonicbld commented Sep 22, 2026

Uh oh!

azure-pipelines Bot commented Sep 22, 2026

baijupn commented Sep 9, 2026 •

edited

Loading