bgpd: fix arg order in bgp_redistribute_add() call #14378

taspelund · 2023-09-08T18:52:59Z

Fixes a call to bgp_redistribute_add() that mixed up the position of args bhtype (blackhole type, enum) and api.distance (RIB admin distance, uint8_t).

Fixes a call to bgp_redistribute_add() that mixed up the position of args bhtype (blackhole type, enum) and api.distance (RIB admin distance, uint8_t). Signed-off-by: Trey Aspelund <[email protected]>

mjstapp

nice!

NetDEF-CI · 2023-09-08T21:49:20Z

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests debian 10 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-14056/test

Topology Tests failed for Topotests debian 10 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9DEB10AMD64/TopotestLogs/log_topotests.txt
Topotests debian 10 amd64 part 9: Unknown Log
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9DEB10AMD64/TopotestDetails/

Topotests Ubuntu 18.04 i386 part 1: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1U18I386-14056/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 1
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO1U18I386/TopotestLogs/log_topotests.txt
Topotests Ubuntu 18.04 i386 part 1: Unknown Log
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO1U18I386/TopotestDetails/

Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)

Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log found

Topotests Ubuntu 18.04 i386 part 8: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO8U18I386-14056/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 8
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO8U18I386/TopotestLogs/log_topotests.txt
Topotests Ubuntu 18.04 i386 part 8: Unknown Log
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO8U18I386/TopotestDetails/

Topotests Ubuntu 18.04 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-14056/test

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9U18AMD64/TopotestLogs/log_topotests.txt
Topotests Ubuntu 18.04 amd64 part 9: Unknown Log
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9U18AMD64/TopotestDetails/

Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-14056/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9U18I386/TopotestLogs/log_topotests.txt
Topotests Ubuntu 18.04 i386 part 9: Unknown Log
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14056/artifact/TOPO9U18I386/TopotestDetails/

Successful on other platforms/tests

Addresssanitizer topotests part 7
Topotests Ubuntu 18.04 i386 part 7
Topotests Ubuntu 18.04 arm8 part 8
Topotests debian 10 amd64 part 4
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 arm8 part 3
Topotests Ubuntu 18.04 amd64 part 3
Addresssanitizer topotests part 5
Topotests Ubuntu 18.04 amd64 part 2
Topotests debian 10 amd64 part 8
Addresssanitizer topotests part 0
Debian 10 deb pkg check
Topotests Ubuntu 18.04 arm8 part 4
Topotests Ubuntu 18.04 amd64 part 4
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 i386 part 6
Topotests Ubuntu 18.04 amd64 part 5
Topotests Ubuntu 18.04 amd64 part 0
Ubuntu 20.04 deb pkg check
Addresssanitizer topotests part 9
Topotests Ubuntu 18.04 amd64 part 7
Topotests debian 10 amd64 part 0
Topotests Ubuntu 18.04 i386 part 3
Topotests Ubuntu 18.04 arm8 part 2
Addresssanitizer topotests part 3
Topotests Ubuntu 18.04 arm8 part 7
Topotests Ubuntu 18.04 arm8 part 5
CentOS 7 rpm pkg check
Addresssanitizer topotests part 6
Topotests debian 10 amd64 part 1
Topotests Ubuntu 18.04 amd64 part 8
Topotests debian 10 amd64 part 7
Static analyzer (clang)
Topotests Ubuntu 18.04 arm8 part 0
Topotests Ubuntu 18.04 i386 part 5
Topotests Ubuntu 18.04 i386 part 0
Topotests debian 10 amd64 part 2
Topotests debian 10 amd64 part 3
Topotests Ubuntu 18.04 amd64 part 6
Topotests Ubuntu 18.04 arm8 part 6
Topotests Ubuntu 18.04 amd64 part 1
Addresssanitizer topotests part 1
Ubuntu 18.04 deb pkg check
Topotests Ubuntu 18.04 i386 part 4
Topotests debian 10 amd64 part 6
Topotests Ubuntu 18.04 arm8 part 1
Debian 9 deb pkg check
Addresssanitizer topotests part 4
Addresssanitizer topotests part 8
Topotests debian 10 amd64 part 5

taspelund · 2023-09-08T23:19:42Z

Looks like a few failures are test_ospf_multi_vrf_bgp_route_leak / test_ospf_kernel_route because the output has a different AD than the test expected (20/ebgp vs 110/ospf):

AssertionError: OSPF IPv4 route mismatch in router "r1": --- Current output
  +++ Expected output
  @@ -1,7 +1,7 @@
   O   10.0.1.0/24 [110/10] is directly connected, r1-eth0, weight 1, XX:XX:XX
   C>* 10.0.1.0/24 is directly connected, r1-eth0, XX:XX:XX
   O>* 10.0.2.0/24 [110/20] via 10.0.20.2, r1-eth1, weight 1, XX:XX:XX
  -B>* 10.0.3.0/24 [110/20] via 10.0.30.3, r1-eth2 (vrf neno), weight 1, XX:XX:XX
  +B>* 10.0.3.0/24 [20/20] via 10.0.30.3, r1-eth2 (vrf neno), weight 1, XX:XX:XX
(16 more lines...)

I suspect that prior to this commit we were copying the AD from the original (redistributed) route blindly, even though it belonged to a different protocol, and the test just matched the old RIB output without paying attention to the AD.

ton31337 · 2023-09-09T08:46:28Z

Oh yeah, good catch :)

taspelund · 2023-09-13T21:24:14Z

Seems like the test is expecting the same thing I would expect to see (BGP-installed entry in RIB should have AD of 20):

[21:26:23] root@ub20:~/frr bgp_distance_good✔➦
 # grep '10.0.3.0/24' tests/topotests/ospf_multi_vrf_bgp_route_leak/r1/zebra-vrf-default.txt
B>* 10.0.3.0/24 [20/20] via 10.0.30.3, r1-eth2 (vrf neno), weight 1, XX:XX:XX

I'll look closer at the distance in the redistributed bgp_path_info and see if that's just blindly being used for RIB install instead of the distances defined for the BGP instance.

Unless other people have differing opinions (@riw777? @donaldsharp? @mjstapp? @ton31337?), I would think that a redistributed route in VRF A that's leaked via BGP into VRF B should be installed into VRF B's RIB with the distance configured for VRF B's BGP rather than the AD from VRF A's RIB entry.

e.g.
OSPF route 10.0.3.0/24 in VRF neno:

O>* 10.0.3.0/24 [110/20] via 10.0.30.3, r1-eth2, weight 1, XX:XX:XX

is then redistributed into BGP (VRF neno) and leaked into VRF Default.

Would we expect 10.0.3.0/24 in VRF Default to have an AD of 20 (BGP) or 110 (OSPF)? I would think 20 (test is correct, need to ID additional needed changes in code), but I'd like to hear what you guys think

ton31337 · 2023-09-18T06:32:31Z

Would we expect 10.0.3.0/24 in VRF Default to have an AD of 20 (BGP) or 110 (OSPF)? I would think 20 (test is correct, need to ID additional needed changes in code), but I'd like to hear what you guys think

I'd expect it to be 20 (as configured), not as it comes from VRF A.

riw777

looks good

riw777 · 2023-09-26T14:20:10Z

Would we expect 10.0.3.0/24 in VRF Default to have an AD of 20 (BGP) or 110 (OSPF)? I would think 20 (test is correct, need to ID additional needed changes in code), but I'd like to hear what you guys think

I'd expect it to be 20 (as configured), not as it comes from VRF A.

I would as well

ton31337 · 2023-10-25T05:09:58Z

@taspelund would like to continue on this?

riw777 · 2023-11-28T13:33:42Z

is anyone still looking at this?

riw777 · 2024-01-02T15:32:34Z

@frrbot autoclose in 1 month

bgpd: fix arg order in bgp_redistribute_add() call

14ba423

Fixes a call to bgp_redistribute_add() that mixed up the position of args bhtype (blackhole type, enum) and api.distance (RIB admin distance, uint8_t). Signed-off-by: Trey Aspelund <[email protected]>

frrbot bot added bgp bugfix labels Sep 8, 2023

github-actions bot added size/XS master labels Sep 8, 2023

mjstapp approved these changes Sep 8, 2023

View reviewed changes

riw777 approved these changes Sep 19, 2023

View reviewed changes

Jafaral added this to the 9.1 milestone Oct 3, 2023

ton31337 removed this from the 9.1 milestone Nov 7, 2023

frrbot bot added the autoclose label Jan 2, 2024

github-actions bot added rebase PR needs rebase and removed autoclose labels Jan 2, 2024

frrbot bot closed this Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bgpd: fix arg order in bgp_redistribute_add() call #14378

bgpd: fix arg order in bgp_redistribute_add() call #14378

taspelund commented Sep 8, 2023

mjstapp left a comment

NetDEF-CI commented Sep 8, 2023

taspelund commented Sep 8, 2023

ton31337 commented Sep 9, 2023

taspelund commented Sep 13, 2023 •

edited

Loading

ton31337 commented Sep 18, 2023

riw777 left a comment

riw777 commented Sep 26, 2023

ton31337 commented Oct 25, 2023

riw777 commented Nov 28, 2023

riw777 commented Jan 2, 2024

bgpd: fix arg order in bgp_redistribute_add() call #14378

bgpd: fix arg order in bgp_redistribute_add() call #14378

Conversation

taspelund commented Sep 8, 2023

mjstapp left a comment

Choose a reason for hiding this comment

NetDEF-CI commented Sep 8, 2023

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

taspelund commented Sep 8, 2023

ton31337 commented Sep 9, 2023

taspelund commented Sep 13, 2023 • edited Loading

ton31337 commented Sep 18, 2023

riw777 left a comment

Choose a reason for hiding this comment

riw777 commented Sep 26, 2023

ton31337 commented Oct 25, 2023

riw777 commented Nov 28, 2023

riw777 commented Jan 2, 2024

taspelund commented Sep 13, 2023 •

edited

Loading