zebra: Get link from the correct netns for vxlan - #8895
Conversation
polychaeta
left a comment
polychaeta
left a comment
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/3a197daf037246e29f9818d19414be2d/raw/08ce2bd16556525e398625a668c26e0c1a25362b/cr_8895_1624328326.diff | git apply
diff --git a/zebra/zebra_vxlan.c b/zebra/zebra_vxlan.c
index 8a7b12c0b..873e4e462 100644
--- a/zebra/zebra_vxlan.c
+++ b/zebra/zebra_vxlan.c
@@ -2032,7 +2032,8 @@ static int zebra_vxlan_handle_vni_transition(struct zebra_vrf *zvrf, vni_t vni,
vni);
/* Find VxLAN interface for this VNI. */
- for (rn = route_top(zvrf->zns->if_table); rn; rn = route_next(rn)) {
+ for (rn = route_top(zvrf->zns->if_table); rn;
+ rn = route_next(rn)) {
ifp = (struct interface *)rn->info;
if (!ifp)
continue;
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: FailedCheckout code: Failed (click for details)Checkout code: No useful log found |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 1 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19738/ This is a comment from an automated CI system. |
|
Rebased against master. |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-361/ This is a comment from an automated CI system. |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests debian 10 amd64 part 9: Failed (click for details)Topotests debian 10 amd64 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7275/artifact/TOPO9DEB10AMD64/ErrorLog/ Topotests debian 10 amd64 part 9: No useful log foundSuccessful on other platforms/tests
|
|
ci:rerun |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7277/ This is a comment from an automated CI system. |
|
@pjdruddy It's long since this PR was open, any comments? @pguibert6WIND Could have look at this maybe? |
|
This PR is stale because it has been open 180 days with no activity. Comment or remove the |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-10088/ This is a comment from an automated CI system. |
|
rebased |
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
|
@chiragshah6 would you like to take a look at this before merging? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
@Mergifyio rebase |
☑️ Nothing to do, the required conditions are not metDetails
|
If VRF backend is netns, bridges and SVIs are in the VRF's netns. Get link information from there, rather than NS_DEFAULT. Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
|
@greptile review |
Greptile SummaryThis PR fixes VxLAN link lookups in
Confidence Score: 5/5Safe to merge — the fix is narrow and consistent with how zif->link is already populated across the codebase. All four changed call-sites are straightforward substitutions: the zif->link pointer is already resolved namespace-correctly by zebra_if_update_link(), and zvrf->zns is the established pattern for VRF-namespace walks. No new code paths are introduced and the remaining NS_DEFAULT usages in the file are unrelated to VXLAN link resolution. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Kernel as Kernel (netns VRF)
participant ZIF as zebra_if (zif)
participant UpdateLink as zebra_if_update_link()
participant VXLAN as zebra_vxlan.c
Kernel->>UpdateLink: interface event (link_ifindex, link_nsid)
UpdateLink->>ZIF: "zif->link_nsid = ns_id"
UpdateLink->>ZIF: "zif->link_ifindex = link_ifindex"
UpdateLink->>ZIF: "zif->link = if_lookup_by_index_per_ns(correct NS)"
Note over VXLAN: Old code (buggy)
VXLAN->>VXLAN: if_lookup_by_index_per_ns(NS_DEFAULT, link_ifindex) - Wrong NS
Note over VXLAN: New code (fixed)
VXLAN->>ZIF: "read zif->link (already resolved to correct NS)"
ZIF-->>VXLAN: pointer to correct interface
Note over VXLAN: Also fixed: vni_transition walk
VXLAN->>VXLAN: "zebra_ns_ifp_walk(zvrf->zns, ...) - Correct VRF namespace"
Reviews (1): Last reviewed commit: "zebra: Get link from the correct netns f..." | Re-trigger Greptile |
☑️ Nothing to do, the required conditions are not metDetails
|
7 participants
If VRF backend is netns, bridges and SVIs are in the VRF's netns.
Get link information from there, rather than NS_DEFAULT.