Add Validation functions BGP BFD for FRR/EOS

[snuffy22]

Made the validation plugin be able to take BFD information similar to the OSPF one.

Works with both EOS and FRR.

Not entirely sure if is correct how I made change to the DUT with enabling BFD in the topology.

(py3-snuff) netlab@netlab:~/snuffy-netlab/tests/integration/bgp.session$ netlab validate
[WARNING] Topology source file(s) have changed since the lab has started
[INFO]    Reading validation tests from /home/netlab/snuffy-netlab/tests/integration/bgp.session/13-bfd.yml
[session_v4] Check IPv4 EBGP sessions with DUT [ node(s): x1 ]
[PASS]       x1: Neighbor 10.1.0.1 (dut) is in state Established
[PASS]       Test succeeded in 0.1 seconds

[bgp_bfd_v4] Check IPv4 EBGP sessions with DUT [ node(s): x1 ]
[PASS]       x1: All specified BGP neighbor parameters have the expected values
[PASS]       Test succeeded in 0.1 seconds

[bfd_v4]     Check BFD peer on X1 [ node(s): x1 ]
[PASS]       Validation succeeded on x1
[PASS]       Test succeeded in 0.1 seconds

[bfd_v4_dut] Check BFD peer on DUT [ node(s): dut ]
[PASS]       Validation succeeded on dut
[PASS]       Test succeeded in 0.2 seconds

[SUCCESS]    Tests passed: 4

[ipspace]

While I'm perfectly fine with checking BFD status with a plugin, I'm wondering whether we need this as the integration test checks whether the tested device starts BFD, so we cannot configure BFD-for-BGP, so we won't get BFD info in the BGP neighbor anyway.

Having said that, it would be nice to have BFD-with-BGP checking functionality, but I think the parameter has to be be three-state: None (not interested in BFD), False (BFD must NOT be running) and True (BFD MUST be running).

[ipspace]

I would reword this along the lines of "No BFD information for BGP peer {n_id}"

[snuffy22]

noted

[ipspace]

Wrong. You see, the point is that DUT runs BFD with one peer but not the other.

[snuffy22]

There is only 1 peer in the bgp.session version of this BFD test topology currently...

[jbemmel]

Took the liberty to add a test for FRRouting/frr#22097

[ipspace]

Took the liberty to add a test for FRRouting/frr#22097

Why on earth would we test every device against an FRR bug that has already been fixed? Nope.

[jbemmel]

Took the liberty to add a test for FRRouting/frr#22097

Why on earth would we test every device against an FRR bug that has already been fixed? Nope.

Because it’s a quick minimal test that checks whether BFD actually works as intended.

If there's a way to only run it if dut is FRR then we could add that if you prefer. I think it might be useful

[ipspace]

Because it’s a quick minimal test that checks whether BFD actually works as intended.

It's not our job to check whether implementations "work as intended". We have to do our best to configure them correctly.

If there's a way to only run it if dut is FRR then we could add that if you prefer. I think it might be useful

If you feel like creating a test suite for FRR, please go ahead. However, although I report everything we find to my FRR contacts, that's not the job of netlab integration tests.

[snuffy22]

I have updated my part, anything else required?

[ipspace]

I have updated my part, anything else required?

My mental energy to review it 🤔 The code looks OK, but...

As I already explained, in the test we want to check whether the tested device (DUT) starts BFD over BGP session. If DUT has BFD configured, but not enabled on the neighbor, and the probe starts a BFD session for its BGP neighbor, the test might succeed (the probe device is happy that it has a BFD session). There's a reason I made the test as convoluted as it looks to you.

The changes to the integration test are thus moot. Tangentially, you can never make any custom changes to the tested device or execute any testing logic on it, or we'd have to implement them for every device we want to test; clearly something that we will NOT do.

Regardless, let's assume we want to add BFD checking functionality to "bgp_neighbor_details" even though it will not be used in the integration test. In that case, it would be nice to have the three-state behavior I mentioned in the initial review:

I think the parameter has to be three-state: None (not interested in BFD), False (BFD must NOT be running) and True (BFD MUST be running).

I can easily add the three-state logic and merge the code, but it will remain unused functionality from the integration test perspective. On a tangential note, we never documented the plugin functions, so even if someone wants to use this, they'd have to dig into the source code to discover it exists.