tests: fix uptime check in test_bgp_default_originate_2links.py

[enkechen-panw]

In bgp_default_originate/test_bgp_default_originate_2links.py,
test_verify_bgp_default_originate_with_default_static_route_p1 uses
the uptime from "show ip route" (zebra) in two places to verify that
a config change to bgp default-originate on one link did not disturb
the bgp 0.0.0.0/0 route received on the other link. Both checks are
fundamentally broken: any change to a member of an ECMP set causes
zebra to reinstall the entire set, resetting the uptime regardless
of whether the monitored path itself changed.

• The first check verified that re-configuring default-originate on
  link-1 did not disturb the path received on link-2.

• The second check verified that removing redistribute static did not
  disturb the default-originate path received on link-1.

Replace both zebra-based uptime checks with two BGP-table checks that
together prove no spurious UPDATE was received on the monitored session:

1. Path attrs comparison: attrs {aspath, origin, metric, nexthop}
   for the peer's path in r2's BGP table (show bgp ipv4/ipv6
   unicast 0.0.0.0/0 json) before and after the config change. Any
   UPDATE carrying different attributes would be caught here.

2. Duplicate-count check: snapshot peer->pcount_dup[afi][safi]
   (exposed as "receivedPrefixDup" in show bgp neighbor PEER json)
   before and after. An UPDATE carrying identical attributes —
   which the attrs comparison would miss — increments this counter.

[greptile-apps]

Greptile Summary

This PR replaces a broken zebra-uptime check (which was invalidated by ECMP reinstalls) with two BGP-layer checks: (1) path-attribute comparison for the link-2 peer's path before/after re-config, and (2) a receivedPrefixDup counter snapshot to catch attribute-identical spurious UPDATEs. The approach is correct and well-motivated.

• _get_dup_count can silently pass on failure: the function returns None on any exception or missing key, and there is no not-None guard before assert dup_after_* == dup_before_*. If the helper silently fails, None == None evaluates to True, making the duplicate-UPDATE half of the check completely invisible without any test failure.

Confidence Score: 4/5

Safe to merge with minor fix — the dup-count check can silently no-op on error, but the path-attrs check still provides meaningful coverage.

The overall approach is correct and a clear improvement over the previous broken check. One P1 issue exists: _get_dup_count returning None on failure makes the dup-count assertions vacuously pass, partially undermining the test's guarantees. Adding a not-None guard would make the test robust.

tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py — _get_dup_count error-return path

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename	Overview
tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py	Replaces a fundamentally broken zebra-uptime check with BGP-level path-attrs and duplicate-UPDATE counter checks; logic is sound but _get_dup_count can silently return None on error, causing the dup-count assertion to pass vacuously.

Sequence Diagram

sequenceDiagram
    participant T as Test
    participant R2 as r2 (BGP)
    participant R1L1 as r1 link-1
    participant R1L2 as r1 link-2

    T->>R2: snapshot: _bgp_stable_attrs_from_peer(link-2, ipv4/ipv6)
    T->>R2: snapshot: _get_dup_count(link-2, ipv4/ipv6)
    Note over T: before = {attrs, dup_count}

    T->>R1L1: re-configure default-originate
    R1L1-->>R2: BGP UPDATE (link-1 path)
    Note over R1L2,R2: link-2 path should be undisturbed

    T->>R2: verify_rib/fib convergence (link-1 + link-2)
    T->>R2: snapshot: _bgp_stable_attrs_from_peer(link-2, ipv4/ipv6)
    T->>R2: snapshot: _get_dup_count(link-2, ipv4/ipv6)
    Note over T: after = {attrs, dup_count}

    T->>T: assert before_attrs == after_attrs
    T->>T: assert dup_after == dup_before

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py
Line: 381-385

Comment:
**Silent pass when `_get_dup_count` fails**

If `vtysh_cmd` raises or JSON parsing fails, the function returns `None`. Because there is no not-`None` guard before the comparison assertions, `assert dup_after_ipv4 == dup_before_ipv4` evaluates to `assert None == None` which is `True` — the test passes silently while the duplicate-UPDATE check is entirely skipped. A missing `addressFamilyInfo` key, wrong peer-IP key, or any transient vtysh error would trigger this path.

```suggestion
    peer_info = output.get(peer_ip, {})
    afi_info = peer_info.get("addressFamilyInfo", {}).get(afi_safi_str, {})
    dup = afi_info.get("receivedPrefixDup")
    return dup if dup is not None else 0
```

And add a guard at the assertion sites, e.g.:

```python
assert dup_before_ipv4 is not None, (
    "{}: failed to read link-2 IPv4 dup count before re-config".format(tc_name)
)
assert dup_before_ipv6 is not None, (
    "{}: failed to read link-2 IPv6 dup count before re-config".format(tc_name)
)
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "tests: fix uptime check in test_bgp_defa..." | Re-trigger Greptile}

[riw777]

looks good

[riw777]

Looks like this needs to be fixed?

  /home/frr/frr/tests/topotests/bgp_link_state/test_bgp_link_state.py:6: DeprecationWarning: invalid escape sequence '\ '```

[enkechen-panw]

Looks like this needs to be fixed?

  /home/frr/frr/tests/topotests/bgp_link_state/test_bgp_link_state.py:6: DeprecationWarning: invalid escape sequence '\ '```

@riw777 That does not seem to be related to this PR. Please advice. Thanks.

[enkechen-panw]

ci:rerun