search

FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.13k stars 1.21k forks source link

isisd: When the ISIS types of the routers do not match on a P2P link, the neighbor status remains UP. #16254

Open zhou-run opened 4 weeks ago

zhou-run commented 4 weeks ago

Test Scenario: RouterA and RouterB are in the same routing domain and have configured a P2P link. RouterA is configured with "is-type level-1" while RouterB is configured with "is-type level-1-2". They establish a level-1 UP neighborship. In this scenario, we expect that when RouterB's configuration is switched to "is-type level-2-only", the neighborship status on both RouterA and RouterB would be non-UP. However, RouterB still shows the neighbor as UP.

Upon receiving a P2P Hello packet, the function process_p2p_hello() is invoked. According to the ISO/IEC 10589 protocol specification, section 8.2.5.2 a) and tables 5 and 7, if the iih->circ_type of the neighbor's hello packet does not match one's own circuit->is_type, we may choose to take no action. When establishing a neighborship for the first time, the neighbor's status can remain in the Initializing state. However, if the neighborship has already been established and one's own circuit->is_type changes, the neighbor's UP status cannot be reset. Therefore, when processing P2P Hello packets, we should be cognizant of changes in our own link adjacency type.

Signed-off-by: zhou-run zhou.run@h3c.com

ton31337 commented 4 weeks ago

@Mergifyio backport dev/10.1 stable/10.0 stable/9.1

mergify[bot] commented 4 weeks ago

backport dev/10.1 stable/10.0 stable/9.1

🟠 Waiting for conditions to match

- [ ] `merged` [📌 backport requirement]
ton31337 commented 4 weeks ago

Could you add a topotest to verify this patch/behavior?

zhou-run commented 3 weeks ago

Could you add a topotest to verify this patch/behavior?

I currently have no plans to add topotest because this is a bug fix, not a new feature.

In addition, there is a failure in the basic test related to OSPF, but I see that topotest does not have any ISIS-related configuration. This should be a false alarm, right? https://ci1.netdef.org/browse/FRR-PULLREQ3-TOPO9U18I386-3819

ton31337 commented 3 weeks ago

I currently have no plans to add topotest because this is a bug fix, not a new feature.

So? How do we ensure we don't hit this bug if we change something later?

zhou-run commented 3 weeks ago

I currently have no plans to add topotest because this is a bug fix, not a new feature.

So? How do we ensure we don't hit this bug if we change something later?

Okay, waiting on the topo test.