ldpd: fix redistribute static issue#235
Open
mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
Open
ldpd: fix redistribute static issue#235mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
mwinter-osr wants to merge 2 commits intoopensourcerouting:masterfrom
Conversation
test the redistribute static beheavior in MPLS cloud with LDP Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
mpls cloud with ldp has misrunning redistribute to ISIS function at LRE side. current beheavior: r2# show mpls ldp binding AF Destination Nexthop Local Label Remote Label In Use ipv4 1.1.1.1/32 3.3.3.3 16 16 yes ipv4 2.2.2.2/32 3.3.3.3 imp-null 17 no ipv4 3.3.3.3/32 3.3.3.3 17 imp-null yes ipv4 12.12.12.12/32 3.3.3.3 18 18 no expected beheavior: r2# show mpls ldp binding AF Destination Nexthop Local Label Remote Label In Use ipv4 1.1.1.1/32 3.3.3.3 16 16 yes ipv4 2.2.2.2/32 3.3.3.3 imp-null 17 no ipv4 3.3.3.3/32 3.3.3.3 17 imp-null yes ipv4 12.12.12.12/32 3.3.3.3 imp-null 18 no fix: align staic with IGP routes Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
Greptile SummaryRemoves the
|
| Filename | Overview |
|---|---|
| ldpd/lde_lib.c | Removes the ZEBRA_ROUTE_STATIC special-case from lde_kernel_update() so that static routes are subject to the same LDP-interface check as IGP routes; stale comment still mentions the removed condition. |
| tests/topotests/ldp_static_redistribute/test_ldp_static_redistribute.py | New topotest covering OSPF and static redistribution through ISIS with LDP; minor issues: copy-paste comment in test_static_redistribute and unclosed file handle in router_compare_json_output. |
| tests/topotests/ldp_static_redistribute/r3/isisd.conf | Orphaned per-daemon config file not loaded by the test (load_frr_config only reads frr.conf); same applies to r3/ldpd.conf and r3/zebra.conf — all three should be removed. |
| tests/topotests/ldp_static_redistribute/r2/frr.conf | PE router configuration: IS-IS, LDP (host-route allocation), and OSPF for CE2 adjacency; straightforward and correct. |
| tests/topotests/ldp_static_redistribute/r1/frr.conf | PE router r1 configuration with IS-IS and LDP; correct interface and address configuration for the test topology. |
Sequence Diagram
sequenceDiagram
participant Zebra as Zebra (r2)
participant LDE as ldpd LDE (r2)
participant R1 as LDP Peer r1
participant R3 as LDP Peer r3
Note over Zebra,LDE: Static route 12.12.12.12/32 via 172.16.1.2 (r2-eth0, non-LDP)
Zebra->>LDE: "ZEBRA_REDISTRIBUTE_IPV4_ADD (route_type=STATIC, ifindex=r2-eth0)"
Note over LDE: lde_kernel_update()
LDE->>LDE: "if_lookup(ldeconf, ifindex=r2-eth0) → NULL (no LDP on r2-eth0)"
Note over LDE: OLD: route_type==STATIC → UNSET F_FEC_NH_NO_LDP → allocate label 18 ❌
Note over LDE: NEW: iface==NULL, not connected → SET F_FEC_NH_NO_LDP → imp-null ✅
LDE->>LDE: lde_fec_outside_mpls_network() → true → imp-null
LDE->>R1: "LDP Label Mapping: 12.12.12.12/32 = imp-null"
LDE->>R3: "LDP Label Mapping: 12.12.12.12/32 = imp-null"
Comments Outside Diff (1)
-
ldpd/lde_lib.c, line 387-390 (link)Stale comment after static-route removal
The comment still says "or a static route" but that condition was just deleted. It now misrepresents the actual logic: the flag is cleared only when the nexthop interface has LDP configured or the nexthop is directly connected.
Prompt To Fix With AI
This is a comment left during a code review. Path: ldpd/lde_lib.c Line: 387-390 Comment: **Stale comment after static-route removal** The comment still says "or a static route" but that condition was just deleted. It now misrepresents the actual logic: the flag is cleared only when the nexthop interface has LDP configured or the nexthop is directly connected. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: ldpd/lde_lib.c
Line: 387-390
Comment:
**Stale comment after static-route removal**
The comment still says "or a static route" but that condition was just deleted. It now misrepresents the actual logic: the flag is cleared only when the nexthop interface has LDP configured or the nexthop is directly connected.
```suggestion
/*
* if LDP is configured on the nexthop interface,
* or the nexthop is directly connected,
* clear flag; otherwise treat fec as outside
* the MPLS network (implicit-null egress)
*/
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/test_ldp_static_redistribute.py
Line: 241
Comment:
**Copy-paste comment error**
The comment says "add redistribute ospf on r2" but this function does the opposite — it removes OSPF redistribution and enables static redistribution.
```suggestion
#remove redistribute ospf and add redistribute static on r2
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/test_ldp_static_redistribute.py
Line: 137-139
Comment:
**File handle not closed**
`open(filename).read()` leaves the file descriptor open until garbage-collection. Other FRR topotests use a context manager. Consider:
```suggestion
with open(filename) as f:
expected = json.load(f)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/r3/isisd.conf
Line: 1-5
Comment:
**Orphaned per-daemon config files for r3**
`r3/isisd.conf`, `r3/ldpd.conf`, and `r3/zebra.conf` are not loaded by the test — `setup_module` calls `load_frr_config()` for every router, which loads only `frr.conf`. All r3 daemon configuration already exists in `r3/frr.conf`. These three files are dead artifacts and should be removed to avoid confusion.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "ldpd: fix redistribute static issue" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if tgen.routers_have_failure(): | ||
| pytest.skip(tgen.errors) | ||
|
|
||
| #add redistribute ospf on r2 |
There was a problem hiding this comment.
The comment says "add redistribute ospf on r2" but this function does the opposite — it removes OSPF redistribution and enables static redistribution.
Suggested change
| #add redistribute ospf on r2 | |
| #remove redistribute ospf and add redistribute static on r2 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/test_ldp_static_redistribute.py
Line: 241
Comment:
**Copy-paste comment error**
The comment says "add redistribute ospf on r2" but this function does the opposite — it removes OSPF redistribution and enables static redistribution.
```suggestion
#remove redistribute ospf and add redistribute static on r2
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+137
to
+139
| filename = "{}/{}/{}".format(CWD, rname, reference) | ||
| expected = json.loads(open(filename).read()) | ||
|
|
There was a problem hiding this comment.
open(filename).read() leaves the file descriptor open until garbage-collection. Other FRR topotests use a context manager. Consider:
Suggested change
| filename = "{}/{}/{}".format(CWD, rname, reference) | |
| expected = json.loads(open(filename).read()) | |
| with open(filename) as f: | |
| expected = json.load(f) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/test_ldp_static_redistribute.py
Line: 137-139
Comment:
**File handle not closed**
`open(filename).read()` leaves the file descriptor open until garbage-collection. Other FRR topotests use a context manager. Consider:
```suggestion
with open(filename) as f:
expected = json.load(f)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1
to
+5
| hostname r3 | ||
| ! log file isisd.log | ||
| ! debug isis adj-packets | ||
| ! debug isis events | ||
| ! debug isis update-packets |
There was a problem hiding this comment.
r3/isisd.conf, r3/ldpd.conf, and r3/zebra.conf are not loaded by the test — setup_module calls load_frr_config() for every router, which loads only frr.conf. All r3 daemon configuration already exists in r3/frr.conf. These three files are dead artifacts and should be removed to avoid confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/topotests/ldp_static_redistribute/r3/isisd.conf
Line: 1-5
Comment:
**Orphaned per-daemon config files for r3**
`r3/isisd.conf`, `r3/ldpd.conf`, and `r3/zebra.conf` are not loaded by the test — `setup_module` calls `load_frr_config()` for every router, which loads only `frr.conf`. All r3 daemon configuration already exists in `r3/frr.conf`. These three files are dead artifacts and should be removed to avoid confusion.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mpls cloud with ldp has misrunning redistribute to ISIS function
at LRE side.
current beheavior:
r2# show mpls ldp binding
AF Destination Nexthop Local Label Remote Label In Use
ipv4 1.1.1.1/32 3.3.3.3 16 16 yes
ipv4 2.2.2.2/32 3.3.3.3 imp-null 17 no
ipv4 3.3.3.3/32 3.3.3.3 17 imp-null yes
ipv4 12.12.12.12/32 3.3.3.3 18 18 no
expected beheavior:
r2# show mpls ldp binding
AF Destination Nexthop Local Label Remote Label In Use
ipv4 1.1.1.1/32 3.3.3.3 16 16 yes
ipv4 2.2.2.2/32 3.3.3.3 imp-null 17 no
ipv4 3.3.3.3/32 3.3.3.3 17 imp-null yes
ipv4 12.12.12.12/32 3.3.3.3 imp-null 18 no
fix: align staic with IGP routes
Signed-off-by: Francois Dumontet francois.dumontet@6wind.com
Original PR20484 by fdumontet6WIND