-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
bgpd: associate correct nexthop when using peer link-local #8961
bgpd: associate correct nexthop when using peer link-local #8961
Conversation
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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 5: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO5DEB10AMD64-19961/test Topology Tests failed for Topotests debian 10 amd64 part 5:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19961/artifact/TOPO5DEB10AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests debian 10 amd64 part 5: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO5DEB10AMD64-19961/test Topology Tests failed for Topotests debian 10 amd64 part 5:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19961/artifact/TOPO5DEB10AMD64/ErrorLog/log_topotests.txt
|
11632b2
to
f89071e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/17990dab281e906f320ec956b85aed3d/raw/3ac068b72d433f6b851b4e008edae1ee98690492/cr_8961_1625131040.diff | git apply
diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index fd46ac564..8156e2e9b 100644
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -753,9 +753,10 @@ bool bgp_zebra_nexthop_set(union sockunion *local, union sockunion *remote,
ifp = if_lookup_by_name(peer->update_if,
peer->bgp->vrf_id);
else
- ifp = if_lookup_by_ipv6_exact(&local->sin6.sin6_addr,
- local->sin6.sin6_scope_id,
- peer->bgp->vrf_id);
+ ifp = if_lookup_by_ipv6_exact(
+ &local->sin6.sin6_addr,
+ local->sin6.sin6_scope_id,
+ peer->bgp->vrf_id);
} else if (peer->update_if)
ifp = if_lookup_by_name(peer->update_if,
peer->bgp->vrf_id);
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.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
f89071e
to
386dd18
Compare
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-19986/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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 Ubuntu 18.04 amd64 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP4U1804AMD64-19993/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 4:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19993/artifact/TP4U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
386dd18
to
57e0f76
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
57e0f76
to
1601fd0
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests Ubuntu 18.04 i386 part 5: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 5: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 8: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 5: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 5: Incomplete(check logs for details)Topotests Ubuntu 18.04 i386 part 8: Incomplete(check logs for details)Addresssanitizer topotests part 4: Incomplete(check logs for details)Successful on other platforms/tests
|
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-20019/ This is a comment from an automated CI system. |
@@ -750,6 +750,14 @@ bool bgp_zebra_nexthop_set(union sockunion *local, union sockunion *remote, | |||
? peer->conf_if | |||
: peer->ifname, | |||
peer->bgp->vrf_id); | |||
else if (peer->update_if) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid case for LL peering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per Donald remark, this should be a valid case, only if update-source command is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking up the correct interface for the LL if you have the peer->update_if
is all that we can do here.
bgpd/bgp_zebra.c
Outdated
ifp = if_lookup_by_name(peer->update_if, | ||
peer->bgp->vrf_id); | ||
else | ||
ifp = if_lookup_by_ipv6_exact( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with getting the correct interface from the peer->update_if
as that is associated with the update-source
cli but given the way LL works in v6 world doing a if_lookup_by_ipv6_exact will more than likely not return the correct interface here and will lead to weird issues and this else statement should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. removed call to ipv6_exact, and kept the check with update-source command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@donaldsharp , please review
When setting bgp configuration using peers referencing link local ipv6 addresses, the bgp should be able to handle incoming bgp connections, and find out the appropriate interface where the connection comes from. ipv6 link local sessions work by using bgp unnumbered interfaces config, but it does not work if we have a shared media with multiple potential link local ipv6 addresses on the network. The fix consists in finding out the appropriate interface, when the local configuration references a link local ipv6 addresses, and the source address used references an interface. below configuration illustrates what can be done then: neighbor fe80::4113:5bba:2b61:b20c remote-as 55 neighbor fe80::4113:5bba:2b61:b20c update-source eth0 note: this change does not solve the ability for such config to create an outgoing connection to remote peer (as the link local ipv6 address config does not indicate which interface to use). Signed-off-by: Philippe Guibert <[email protected]>
1601fd0
to
abe6805
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-FRRPULLREQ-20175/ This is a comment from an automated CI system. |
@Mergifyio backport stable/8.0 |
Command
|
bgpd: associate correct nexthop when using peer link-local (backport #8961)
When setting bgp configuration using peers referencing link local
ipv6 addresses, the bgp should be able to handle incoming bgp
connections, and find out the appropriate interface where the
connection comes from.
ipv6 link local sessions work by using bgp unnumbered interfaces
config, but it does not work if we have a shared media with
multiple potential link local ipv6 addresses on the network.
The fix consists in finding out the appropriate interface, when
the local configuration references a link local ipv6 addresses.
note: this change does not solve the ability for such config to
create an outgoing connection to remote peer (as the link local
ipv6 address config does not indicate which interface to use).
Signed-off-by: Philippe Guibert [email protected]