-
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
nhrp: NAT fixes #8240
nhrp: NAT fixes #8240
Conversation
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Excellent work @reubendowle, thanks for fixing nhrp! |
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 arm8 part 7: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 7: No useful log foundTopotests Ubuntu 16.04 i386 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U16I386-17598/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO6U16I386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO7U18AMD64-17598/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 7:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO7U18AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-17598/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 arm8 part 7: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 7: No useful log foundTopotests Ubuntu 16.04 i386 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U16I386-17598/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO6U16I386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 7: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO7U18AMD64-17598/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 7:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO7U18AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-17598/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17598/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
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.
before going further, I would like to get a source of the specification you used to do nat extension.
my reference is https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/sec_conn_dmvpn/configuration/15-mt/sec-conn-dmvpn-15-mt-book/sec-conn-dmvpn-dt-spokes-b-nat.html.
so there are some items that are clearly not covered by this documentation.
if you reproduce it with a cisco version, thanks to communicate cisco version used.
nhrpd/nhrp_peer.c
Outdated
if (!cie) | ||
goto err; | ||
} | ||
} else { |
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 don't understand why you add some more information on the hub.
on https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/sec_conn_dmvpn/configuration/15-mt/sec-conn-dmvpn-15-mt-book/sec-conn-dmvpn-dt-spokes-b-nat.html, it simply asks to add nhrp extension if needed, but not to fill in nhrp extension.
"
The hub receives the resolution request. If the spoke is behind a NAT device and there is no NAT extension, then the hub adds a NAT extension before forwarding this extension to the next node (spoke or next hop server) along the path.
"
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.
It looks like @reubendowle missed one change in the commit. I'll discuss with him. The actual change in hub is
"The hub receives the resolution request. If the spoke is behind a NAT device and there is no NAT extension, then the hub adds a NAT extension before forwarding this extension to the next node (spoke or next hop server) along the path. However, if spoke already includes NAT extension header then it is just copied across and no changes are made to the extension header received from spoke"
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.
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.
before going further, I would like to get a source of the specification you used to do nat extension.
my reference is https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/sec_conn_dmvpn/configuration/15-mt/sec-conn-dmvpn-15-mt-book/sec-conn-dmvpn-dt-spokes-b-nat.html.so there are some items that are clearly not covered by this documentation.
From our research, cisco does not publically disclose the exact details of how the NAT extension should work. This is a proprietary extension (in fact the extension number 9 is actually defined in RFC2735 for a completely different purpose https://tools.ietf.org/html/rfc2735#section-4.2, so technically the NAT extension is violation of RFC. But that said the NAT extension is widely used and is required for interoperability in real world networks.
The main source of our information is reverse engineering of the protocol through observing packets from Cisco devices in wireshark.
if you reproduce it with a cisco version, thanks to communicate cisco version used.
We have tested against a number of different Cisco devices. These are the devices in our lab, although I think Amol has some others in his setup:
- Cisco 1921 with IOS 15.7(3)M7
- Cisco 2951 with IOS 15.6(3)M2
- Cisco 2901 with IOS 15.6(3)M2
- Cisco 819 with Version 15.6(3)M0a
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 dit a test with an our nhrp nat implementation. I followed the cisco link spec and expect that it will work.
- frr implementation at hub side not behind nat device : interact ok with spoke other implementation (1)
- frr implementation at spoke side not behind nat device: interact ok with spoke other implementation (2)
for (2), I could see that resolution reply is sent back via the hub, whereas on [0] it is written that resolution reply should be sent directly. So I guess the cisco implementations you used do not send directly resolution reply ?
Spoke A parses the NHRP NAT-extension and builds a tunnel using Spoke B’s post-NAT address and replies directly to Spoke B.
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 dit a test with an our nhrp nat implementation. I followed the cisco link spec and expect that it will work.
- frr implementation at hub side not behind nat device : interact ok with spoke other implementation (1)
- frr implementation at spoke side not behind nat device: interact ok with spoke other implementation (2)
for (2), I could see that resolution reply is sent back via the hub, whereas on [0] it is written that resolution reply should be sent directly. So I guess the cisco implementations you used do not send directly resolution reply ?
Spoke A parses the NHRP NAT-extension and builds a tunnel using Spoke B’s post-NAT address and replies directly to Spoke B.
Well, I am aware of this and this is not easy to address. The problem is when spoke receives the Resolution-Request, then nhrpd asks Zebra to update the routing table / neighbor table accordingly. However, before Zebra gets a chance to update all this, the Resolution-Reply has already left the Spoke using old routing entries - as a result the reply goes via hub. The only way to address this is to wait for Zebra to complete all routing table updates and then only send Resolution-Reply. We thought it's not worth the effort involved ....
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 dit a test with an our nhrp nat implementation. I followed the cisco link spec and expect that it will work.
- frr implementation at hub side not behind nat device : interact ok with spoke other implementation (1)
- frr implementation at spoke side not behind nat device: interact ok with spoke other implementation (2)
for (2), I could see that resolution reply is sent back via the hub, whereas on [0] it is written that resolution reply should be sent directly. So I guess the cisco implementations you used do not send directly resolution reply ?
[0] https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/sec_conn_dmvpn/configuration/15-mt/sec-conn-dmvpn-15-mt-book/sec-conn-dmvpn-dt-spokes-b-nat.htmlSpoke A parses the NHRP NAT-extension and builds a tunnel using Spoke B’s post-NAT address and replies directly to Spoke B.
Well, I am aware of this and this is not easy to address. The problem is when spoke receives the Resolution-Request, then nhrpd asks Zebra to update the routing table / neighbor table accordingly. However, before Zebra gets a chance to update all this, the Resolution-Reply has already left the Spoke using old routing entries - as a result the reply goes via hub. The only way to address this is to wait for Zebra to complete all routing table updates and then only send Resolution-Reply. We thought it's not worth the effort involved ....
ok, you mean the NBMA address of remote spoke may not be reachable directly. But don't you have spokes with default route, so that all remote spokes should be reachable directly ? I use following snipped of code to send to spoke instead of hub:
-void nhrp_peer_send(struct nhrp_peer *p, struct zbuf *zb)
+static void nhrp_peer_send_to(struct nhrp_peer *p, struct zbuf *zb, union sockunion *target)
{
char buf[2][256];
struct nhrp_vrf *nhrp_vrf;
+ union sockunion *to;
nhrp_packet_debug(zb, "Send");
@@ -336,33 +337,42 @@ void nhrp_peer_send(struct nhrp_peer *p, struct zbuf *zb)
zlog_err("%s(): nhrp_vrf context not found", __func__);
return;
}
+ if (target)
+ to = target;
+ else
+ to = &p->vc->remote.nbma;
debugf(NHRP_DEBUG_KERNEL, "PACKET: Send %s -> %s",
sockunion2str(&p->vc->local.nbma, buf[0], sizeof buf[0]),
- sockunion2str(&p->vc->remote.nbma, buf[1], sizeof buf[1]));
+ sockunion2str(to, buf[1], sizeof buf[1]));
os_sendmsg(zb->head, zbuf_used(zb), p->ifp->ifindex,
- sockunion_get_addr(&p->vc->remote.nbma),
- sockunion_get_addrlen(&p->vc->remote.nbma),
+ sockunion_get_addr(to), sockunion_get_addrlen(to),
nhrp_vrf->nhrp_socket_fd);
zbuf_reset(zb);
}
and that patch
@@ -507,7 +536,14 @@ static void nhrp_handle_resolution_req(struct nhrp_packet_parser *pp)
}
nhrp_packet_complete(zb, hdr);
- nhrp_peer_send(peer, zb);
+
+ /* if post-NAT set, reply directly to post-NAT ip address */
+ if (sockunion_family(&cie_nat) != AF_UNSPEC)
+ nbma_addr = &cie_nat;
+ else
+ nbma_addr = NULL;
+ nhrp_peer_send_to(peer, zb, nbma_addr);
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 it ok to defer this change to a new pull-request? I will need to test it and currently not in position to test it
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.
This idea looks good. Although this patch does not fit with code in this pull- there is no cie_nat variable. I think the reply would need to go direct to the already set nbma_addr variable which has chosen the most likely true nbma, from one of nat extension, ipsec parameters or the claimed nbma.
I don't have the time to check this out though. So as @amollad said, can we split this off. We are confident the code as is works in practice, as the hub always forwards the replies. Maybe a new issue could be created for this defect?
9e3a806
to
f0475ee
Compare
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: FailedUbuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build:
Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1804PPC64LEBUILD/config.status/config.status Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build:
Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI021BUILD/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build:
Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1604I386/config.status/config.status Ubuntu 18.04 arm7 build: Failed (click for details)Make failed for Ubuntu 18.04 arm7 build:
Ubuntu 18.04 arm7 build: Unknown Log <config.log.gz> Ubuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build:
Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI014BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
Ubuntu 20.04 amd64 build: Unknown Log <config.log.gz> Debian 8 amd64 build: Failed (click for details)Debian 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI008BLD/config.log/config.log.gzMake failed for Debian 8 amd64 build:
Debian 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI008BLD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/F29BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Debian 10 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/DEB10BUILD/config.log/config.log.gzMake failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/DEB10BUILD/config.status/config.status Ubuntu 16.04 arm8 build: Failed (click for details)Make failed for Ubuntu 16.04 arm8 build:
Ubuntu 16.04 arm8 build: Unknown Log <config.log.gz> Ubuntu 18.04 arm8 build: Failed (click for details)Ubuntu 18.04 arm8 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U18ARM8BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U18ARM8BUILD/config.log/config.log.gzMake failed for Ubuntu 18.04 arm8 build:
Ubuntu 18.04 amd64 build: Failed (click for details)Make failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1804AMD64/config.status/config.status Ubuntu 16.04 arm7 build: Failed (click for details)Ubuntu 16.04 arm7 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI101BUILD/config.status/config.status Ubuntu 16.04 arm7 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI101BUILD/config.log/config.log.gzMake failed for Ubuntu 16.04 arm7 build:
CentOS 8 amd64 build: Failed (click for details)CentOS 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CENTOS8BUILD/config.status/config.statusMake failed for CentOS 8 amd64 build:
CentOS 8 amd64 build: Unknown Log <config.log.gz> Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsUbuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build:
Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1804PPC64LEBUILD/config.status/config.status Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build:
Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI021BUILD/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build:
Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1604I386/config.status/config.status Ubuntu 18.04 arm7 build: Failed (click for details)Make failed for Ubuntu 18.04 arm7 build:
Ubuntu 18.04 arm7 build: Unknown Log <config.log.gz> Ubuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build:
Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI014BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build:
Ubuntu 20.04 amd64 build: Unknown Log <config.log.gz> Debian 8 amd64 build: Failed (click for details)Debian 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI008BLD/config.log/config.log.gzMake failed for Debian 8 amd64 build:
Debian 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI008BLD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build:
Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/F29BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Debian 10 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/DEB10BUILD/config.log/config.log.gzMake failed for Debian 10 amd64 build:
Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/DEB10BUILD/config.status/config.status Ubuntu 16.04 arm8 build: Failed (click for details)Make failed for Ubuntu 16.04 arm8 build:
Ubuntu 16.04 arm8 build: Unknown Log <config.log.gz> Ubuntu 18.04 arm8 build: Failed (click for details)Ubuntu 18.04 arm8 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U18ARM8BUILD/config.status/config.status Ubuntu 18.04 arm8 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U18ARM8BUILD/config.log/config.log.gzMake failed for Ubuntu 18.04 arm8 build:
Ubuntu 18.04 amd64 build: Failed (click for details)Make failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/U1804AMD64/config.status/config.status Ubuntu 16.04 arm7 build: Failed (click for details)Ubuntu 16.04 arm7 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI101BUILD/config.status/config.status Ubuntu 16.04 arm7 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CI101BUILD/config.log/config.log.gzMake failed for Ubuntu 16.04 arm7 build:
CentOS 8 amd64 build: Failed (click for details)CentOS 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17643/artifact/CENTOS8BUILD/config.status/config.statusMake failed for CentOS 8 amd64 build:
CentOS 8 amd64 build: Unknown Log <config.log.gz>
|
f0475ee
to
acff149
Compare
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-FRRPULLREQ-17645/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
CLANG Static Analyzer Summary
1 Static Analyzer issues remaining.See details at |
224b9e5
to
7a4fbcb
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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-FRRPULLREQ-17647/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
|
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-17648/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
|
7a4fbcb
to
4195ee7
Compare
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 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-17679/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17679/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO6U18AMD64-17679/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 6:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17679/artifact/TOPO6U18AMD64/ErrorLog/log_topotests.txt
|
Is there something wrong with the CI system? This failure seems unrelated to my changes. |
4195ee7
to
b9c1deb
Compare
Continuous 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 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17898/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17898/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/TOPOU1604/ErrorLog/log_topotests.txt Addresssanitizer topotests part 0: Failed (click for details)Addresssanitizer topotests part 0: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/ASAN3/AddressSanitizerError/AddressSanitzer.txt Addresssanitizer topotests part 0: No useful log foundTopotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17898/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17898/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/TOPOI386/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17898/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17898/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/TOPOU1604/ErrorLog/log_topotests.txt Addresssanitizer topotests part 0: Failed (click for details)Addresssanitizer topotests part 0: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/ASAN3/AddressSanitizerError/AddressSanitzer.txt Addresssanitizer topotests part 0: No useful log foundTopotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17898/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17898/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17898/artifact/TOPOI386/ErrorLog/log_topotests.txt
|
Continuous 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 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17902/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17902/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17902/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17902/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17902/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17902/artifact/TOPOI386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17902/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17902/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17902/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17902/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17902/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17902/artifact/TOPOI386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 arm8 part 4: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 4: No useful log found
|
40ea16d
to
925794e
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous 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 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17943/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17943/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17943/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17943/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17943/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17943/artifact/TOPOI386/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-17943/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 0 Topotests Ubuntu 18.04 arm8 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U18ARM8-17943/test Topology Tests failed for Topotests Ubuntu 18.04 arm8 part 0 Topotests Ubuntu 16.04 amd64 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-17943/test Topology Tests failed for Topotests Ubuntu 16.04 amd64 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17943/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotests Ubuntu 16.04 i386 part 0: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-17943/test Topology Tests failed for Topotests Ubuntu 16.04 i386 part 0:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17943/artifact/TOPOI386/ErrorLog/log_topotests.txt
|
I have an other remark on some more changes about debug display message and formatting.
|
this should be ok, I believe. @reubendowle will push the changes |
@pguibert6WIND Please go ahead and push that change |
@pguibert6WIND the topotest that I pulled in is causing issues. Firstly it seems to be failing with "EnvironmentError: you must run pytest with '-s' in order to use mininet CLI". Secondly it is triggering a crash. There is a bug where nhrpd crashes when an interface is deleted. This is a long standing issue, and I fixed a lot of the issues with ee72f0a but it looks like there are still some issues remaining. I don't have the time right now to chase this down, and I don't feel it is related to this pull request as such. I think the topotest could be made to work by deleting the nhrp configuration elements before the interface is removed. But again I don't have the time to look into it. So if you are ok, I will remove the topotest from this pull request. |
From my perspective, I agree to postpone the following changes:
For the topotest crash, a similar topotest ran ok in #8145. I did not test it on upstream master. I will create a separate pr for the topotest test to see if it fails. If it fails, the problem is unrelated to nat changes. For the formatting changes proposed, the code is straighforward and should not need additional testing. |
#8354 fails with current code, but not with 8145
|
Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com>
Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com>
Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com>
925794e
to
0551aea
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-18041/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
|
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.
LGTM
@reubendowle , can you take care of the remaining style warnings please ? I see a few long lines and an if missing mandatory braces still. Other than that this should be good to go. |
Also cleanup some minor style issues Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com>
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!
- One of your commits has a missing or badly formatted
Signed-off-by
line; we can't accept your contribution until all of your commits have one - One of your commits does not have a blank line between the summary and body; this will break
git log --oneline
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.
Signed-off-by: Reuben Dowle <reuben.dowle@4rf.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
73c7bf9
to
ba113ac
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-18128/ This is a comment from an automated CI system. |
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-18132/ This is a comment from an automated CI system. |
This series of changes fixes a bunch of errors in handling NAT extensions, causing multiple problems. Especially when inter operating with Cisco routers, the previous code was very broken.
Now NAT handling is tested and working with any combination of: