Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix IPv4 routes with IPv6 link local next hops installed in FPM #8740

Merged
merged 5 commits into from
Jan 18, 2022
Merged

Fix IPv4 routes with IPv6 link local next hops installed in FPM #8740

merged 5 commits into from
Jan 18, 2022

Conversation

nkelapur
Copy link
Contributor

Why I did it

Currently IPv4 routes with IPv6 link local next hops are not properly installed in FPM.
Reason is the netlink decoding truncates the ipv6 LL address to 4 byte
ipv4 address.

Ex : fe80:: is directly converted to ipv4 and it results in 254.128.0.0
as next hop for below routes

show ip route
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
F - PBR, f - OpenFabric,

  • selected route, * - FIB route, q - queued, r - rejected, b - backup

B>* 2.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 5.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 10.1.0.2/32 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight
1, 02:22:26

How I did it

Hence this fix converts the ipv6-LL address to ipv4-LL (169.254.0.1)
address before sending it to FPM. This is inline with how these types of
routes are currently programmed into kernel.

How to verify it

Setup BGP peer over ipv6 link local interface address and learn routes.
Check the APP_DB entry for the routes should contain ipv4 Linl Local address instead of incomplete ipv6 address.

APPL_DB hgetall ROUTE_TABLE:2.2.2.2/32
ifname
Ethernet64
nexthop
169.254.0.1 >> ipv4 Link Local address

Signed-off-by: Nikhil Kelapure [email protected]

@nkelapur
Description: Currently IPv4 routes with IPv6 link local next hops are
fae1092 
not properly installed in FPM.
Reason is the netlink decoding truncates the ipv6 LL address to 4 byte
ipv4 address.

Ex : fe80:: is directly converted to ipv4 and it results in 254.128.0.0
as next hop for below routes

show ip route
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
F - PBR, f - OpenFabric,
> - selected route, * - FIB route, q - queued, r - rejected, b - backup

B>* 2.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 5.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 10.1.0.2/32 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight
1, 02:22:26

Hence this fix converts the ipv6-LL address to ipv4-LL (169.254.0.1)
address before sending it to FPM. This is inline with how these types of
routes are currently programmed into kernel.

Signed-off-by: Nikhil Kelapure <[email protected]>
@nkelapur nkelapur requested a review from lguohan as a code owner September 13, 2021 16:55
@nkelapur
Copy link
Contributor Author

This PR #8740 along with PR sonic-net/sonic-swss#1903 fixes issue #8606

@nkelapur
Copy link
Contributor Author

retest this please

@nkelapur
Copy link
Contributor Author

FRR PR for the change is already merged to master FRRouting/frr#9602

@nkelapur
Copy link
Contributor Author

retest this please

@nkelapur
Copy link
Contributor Author

retest vs please

@nkelapur
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator

This deviates from the HLD design here sonic-net/SONiC#625 (comment)
Can you please explain the reason?
Can you please update the HLD with the design changes?

@nkelapur
Copy link
Contributor Author

nkelapur commented Oct 5, 2021

This deviates from the HLD design here Azure/SONiC#625 (comment) Can you please explain the reason? Can you please update the HLD with the design changes?

To Make the design in sync with kernel and FRR, it was decided to implement it this way. Will update the HLD as required.

@liat-grozovik
Copy link
Collaborator

@dgsudharsan could you please help to review?

dgsudharsan
dgsudharsan previously approved these changes Oct 19, 2021
View reviewed changes
@dgsudharsan
Copy link
Collaborator

@nkelapur Can you please sync to the latest? This PR has merge conflicts.

@nkelapur
Description: Currently IPv4 routes with IPv6 link local next hops are
1675cf8 
not properly installed in FPM.
Reason is the netlink decoding truncates the ipv6 LL address to 4 byte
ipv4 address.

Ex : fe80:: is directly converted to ipv4 and it results in 254.128.0.0
as next hop for below routes

show ip route
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
F - PBR, f - OpenFabric,
> - selected route, * - FIB route, q - queued, r - rejected, b - backup

B>* 2.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 5.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 10.1.0.2/32 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight
1, 02:22:26

Hence this fix converts the ipv6-LL address to ipv4-LL (169.254.0.1)
address before sending it to FPM. This is inline with how these types of
routes are currently programmed into kernel.

Signed-off-by: Nikhil Kelapure <[email protected]>
@AkhileshSamineni AkhileshSamineni mentioned this pull request Dec 14, 2021
Merged
@liat-grozovik liat-grozovik added the Request for 202111 Branch For PRs being requested for 202111 branch label Jan 3, 2022
shi-su
shi-su reviewed Jan 3, 2022
View reviewed changes
src/sonic-frr/patch/0011-Ipv4-Link-local-address-for-Ipv6-linl-local-nexthop.patch
static void zfpm_mac_info_del(struct fpm_mac_info_t *fpm_mac);

+static const char ipv4_ll_buf[16] = "169.254.0.1";
+union g_addr ipv4ll_gateway;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose for this g_addr ipv4ll_gateway?

@dgsudharsan
Copy link
Collaborator

@lguohan @prsunny @shi-su Here is the link to the upstream FRR patch

FRRouting/frr#9602

Commit ID: 316d2d52a25a4047f08952dd4aca6192be6240c7

@dgsudharsan dgsudharsan requested a review from shi-su January 12, 2022 04:01
@prsunny prsunny merged commit 9b93749 into sonic-net:master Jan 18, 2022
@adyeung adyeung mentioned this pull request Jan 19, 2022
Closed
judyjoseph pushed a commit that referenced this pull request Jan 23, 2022
@nkelapur @judyjoseph
Fix IPv4 routes with IPv6 link local next hops installed in FPM (#8740)
907629e 
* Description: Currently IPv4 routes with IPv6 link local next hops are
not properly installed in FPM.
Reason is the netlink decoding truncates the ipv6 LL address to 4 byte
ipv4 address.

Ex : fe80:: is directly converted to ipv4 and it results in 254.128.0.0
as next hop for below routes

show ip route
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
F - PBR, f - OpenFabric,
> - selected route, * - FIB route, q - queued, r - rejected, b - backup

B>* 2.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 5.1.0.0/16 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight 1,
02:22:26
B>* 10.1.0.2/32 [200/0] via fe80::268a:7ff:fed0:d40, Ethernet0, weight
1, 02:22:26

Hence this fix converts the ipv6-LL address to ipv4-LL (169.254.0.1)
address before sending it to FPM. This is inline with how these types of
routes are currently programmed into kernel.

Signed-off-by: Nikhil Kelapure <[email protected]>
@arlakshm
Copy link
Contributor

arlakshm commented Mar 1, 2022

PR cherry-pick is failing 202106. Please create a new PR for 202106

@dgsudharsan
Copy link
Collaborator

@nkelapur Can you please create a new PR?

@nkelapur
Copy link
Contributor Author

nkelapur commented Mar 1, 2022 via email

@dgsudharsan
Copy link
Collaborator

@nkelapur The feature was introduced in 202106 branch and I believe in order for it to work, the fix is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shi-su shi-su shi-su approved these changes

@dgsudharsan dgsudharsan dgsudharsan approved these changes

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

Assignees
No one assigned
Labels
Included in 202111 Branch Request for 202106 Branch Request for 202111 Branch For PRs being requested for 202111 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nkelapur @mssonicbld @dgsudharsan @liat-grozovik @arlakshm @shi-su @prsunny @judyjoseph