Donald Sharp [Tue, 15 Oct 2019 01:09:55 +0000 (21:09 -0400)]
bgpd: Be careful about displaying vni's as labels.
When a type 2/3 or 5 route is received, verified and the
resulting route generated is pushed into the appropriate vrf
the vni's associated with the route are also passed in.
This is showing up as a Remote label when you dump
the route in bgp:
BGP routing table entry for 0.0.0.0/0^M
Paths: (1 available, best #1, table third)
Advertised to non peer-group peers:
10.10.120.22
42001 42005 42006 42055
10.10.120.22 from 10.10.120.22 (10.10.255.193)
Origin IGP, valid, external, bestpath-from-AS 42001, best
Remote label: 62750
AddPath ID: RX 0, TX 2
Last update: Fri Oct 11 12:59:56 2019
The `Remote label: 62750` is the mpls label version of the
vni passed in. This is meaningless and confusing to the end
user. Do not display this information.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Tue, 15 Oct 2019 00:44:56 +0000 (20:44 -0400)]
bgpd: Properly lock parent node for type4 routes
When creating a bgp_path_info for a type 4 route the pi->extra->parent
and the route node for the originating table were not being locked
properly. This will prevent BGP from not properly cleaning up
the data structures on cleanup.
Possibly every one of the functions that we use to create the
new bgp_path_info's should use an abstracted version of this code,
but I am unsure at this point in time if a type 4 should use the same
or not.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Quentin Young [Mon, 14 Oct 2019 16:09:36 +0000 (16:09 +0000)]
bgpd: move assert out of error case
bgp_process_packets has an assert to make sure an appropriate amount of
working space in the input buffer has been freed up for future reads.
However, this assert shouldn't be made when we have encountered an error
that's going to tear down the session, because in this case we may not
be able to process the full contents of the input buffer.
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Rafael Zalamena [Fri, 11 Oct 2019 23:15:46 +0000 (20:15 -0300)]
lib: use `prefix` for yang get prefix wrapper
This change fixes a static analyzer warning and should also make us
safer when using this function. At the moment the code that triggered
the warning is the only one that uses this function.
Passing anything other than `struct prefix` to `str2prefix` function is
dangerous, because the structure might be smaller than expected and we
might have an buffer overflow.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Renato Westphal [Thu, 10 Oct 2019 01:20:26 +0000 (22:20 -0300)]
lib: reduce memory allocation when processing large config transactions
Remove the xpath field from the nb_config_cb structure in order
to reduce its size. This allows the northbound to spend less time
allocating memory during the processing of large configuration
transactions.
To make this work, use yang_dnode_get_path() to obtain the xpath
from the dnode field whenever necessary.
Renato Westphal [Thu, 10 Oct 2019 02:24:04 +0000 (23:24 -0300)]
lib: fix processing of the 'apply_finish' callbacks
Commit 6b5d6e2dbc88 changed how we order configuration callbacks
and introduced a regression in the processing of the 'apply_finish'
callbacks. Fix this.
Renato Westphal [Thu, 10 Oct 2019 01:17:08 +0000 (22:17 -0300)]
lib: optimize loading of the startup configuration
Load the startup configuration directly into the CLI shared candidate
configuration instead of loading it into a private candidate
configuration. This way we don't need to initialize the shared
candidate separately later as a copy of the running configuration,
which is a potentially expensive operation.
Also, make the northbound process SIGHUP correctly even when --tcli
is not used.
Renato Westphal [Sun, 6 Oct 2019 04:16:47 +0000 (01:16 -0300)]
lib: avoid expensive operations when editing a candidate config
nb_candidate_edit() was calling both the lyd_schema_sort() and
lyd_validate() functions whenever a new node was added to the
candidate configuration. This was done to ensure the candidate
is always ready to be displayed correctly (libyang only creates
default child nodes during the validation process, and data nodes
aren't guaranteed to be ordered by default).
The problem is that the two aforementioned functions are too
expensive to be called in the northbound hot path. Instead, it makes
more sense to call them only before displaying the configuration
(in which case a recursive sort needs to be done). Introduce the
nb_cli_show_config_prepare() to achieve that purpose.
Renato Westphal [Sun, 6 Oct 2019 00:20:28 +0000 (21:20 -0300)]
lib: remove expensive error handling in the northbound CLI client
The nb_cli_apply_changes() function was creating a full copy of
the candidate configuration before editing it. This excerpt from
the northbond documentation explains why this was being done:
"NOTE: the nb_cli_cfg_change() function clones the candidate
configuration before actually editing it. This way, if any error
happens during the editing, the original candidate is restored to
avoid inconsistencies. Either all changes from the configuration
command are performed successfully or none are. It's like a
mini-transaction but happening on the candidate configuration
(thus the northbound callbacks are not involved)".
The problem is that this kind of error handling is just too
expensive. A command should never fail to edit the candidate
configuration unless there's a bug in the code (e.g. when the
CLI wrapper command passes an integer value that YANG rejects due
to a "range" statement). In such cases, a command might fail to
be applied or applied only partially if it edits multiple YANG
nodes. When that happens, just log an error to make the operator
aware of the problem, but otherwise ignore it instead of rejecting
the command and restoring the candidate to its previous state. We
shouldn't add an extreme overhead to the northbound CLI client only
to handle errors that should never happen in practice.
Renato Westphal [Sat, 5 Oct 2019 23:52:36 +0000 (20:52 -0300)]
lib: optimize VTY_CHECK_XPATH
There's no need to check for removed configuration objects in the
following two cases:
* A private candidate configuration is being edited;
* The startup configuration is being read.
Rafael Zalamena [Fri, 11 Oct 2019 19:13:24 +0000 (16:13 -0300)]
bfdd: simplify session observers code
Don't be selective about what to observe, always observe all possible
aspects of the session that may change on run-time (i.e. bind address,
interface and VRF existence).
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Rafael Zalamena [Fri, 11 Oct 2019 16:12:26 +0000 (13:12 -0300)]
bfdd: set session down after disabling it
If a session is no longer able to send/receive packets, it is very
likely it will be down in a few milliseconds so lets speed up the
process and correctly mark it as down.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
zebra: entries can be pushed in tables, under vrf netns backend
initially, vrf backend if vrf-lite, and a specific table identifier is
associated to a vrf. here, with netns vrf backend, there is no specific
table assigned to except default routing table. use the passed table_id
parameter in zapi api, and apply it to the entry to be pushed in, or to
be removed.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
bgpd: withdraw fib entry on appropriate table identifier
There are cases where the table identifier is set on a bgp entry, mainly
due to route-map, and associate fib entry needs to be removed.
This change encompasses also the route-map reconfiguration that leads to
removing the previous entry, whereas bgp update had been triggered (
this happens when software inbound reconfiguration is handled).
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Philippe Guibert [Mon, 29 Apr 2019 13:26:01 +0000 (15:26 +0200)]
bgpd: ability to export prefixes entries to a kernel table identifier
this table identifier can be used for policy routing. incoming entries
are locally exported to that local table identifier.
note that so that the user applies the new table identifier to all
entries, the user should flush local tables first.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Effectively prd or vnc.import.rd are `struct prefix_rd` which
are being typecast to a `struct prefix`. Not a big deal except commit 1315d74de97be2944d7b005b2f9a50e9ae5eff4d modified the prefix_cmp
function to allow for a sorted prefix_cmp. In prefix_cmp
we were looking at the offset and shift. In the case
of vnc we were passing a prefix length of 64 which is the exact length of
the remaining data structure for struct prefix_rd. So we calculated
a offset of 8 and a shift of 0. The data structures for the prefix
portion happened to be equal to 64 bits of data. So we checked that
with the memcmp got a 0 and promptly read off the end of the data
structure for the numcmp. The fix is if shift is 0 that means thei
the memcmp has checked everything and there is nothing to do.
Please note: We will still crash if we set the prefixlen > then
~312 bits currently( ie if the prefixlen specifies a bit length
longer than the prefix length ). I do not think there is
anything to do here( nor am I sure how to correct this either )
as that we are going to have some severe problems when we muck
up the prefixlen.
Fixes: #5025 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Thu, 10 Oct 2019 00:19:56 +0000 (20:19 -0400)]
bgpd: When creating extra from stack ensure it is zero'ed out
BGP code assumes that the extra data is zero'ed out. Ensure that we
are not leaving any situation that the data on the stack is actually all
0's when we pass it around as a pointer later.
Please note in issue #5025, Lou reported a different valgrind
issue, which is not the same issue:
==7313== Conditional jump or move depends on uninitialised value(s)
==7313== at 0x181F9F: subgroup_announce_check (bgp_route.c:1555)
==7313== by 0x1A112B: subgroup_announce_table (bgp_updgrp_adv.c:641)
==7313== by 0x1A1340: subgroup_announce_route (bgp_updgrp_adv.c:704)
==7313== by 0x1A13E3: subgroup_coalesce_timer (bgp_updgrp_adv.c:331)
==7313== by 0x4EBA615: thread_call (thread.c:1531)
==7313== by 0x4E8AC37: frr_run (libfrr.c:1052)
==7313== by 0x1429E0: main (bgp_main.c:486)
==7313==
==7313== Conditional jump or move depends on uninitialised value(s)
==7313== at 0x201C0E: rfapi_vty_out_vncinfo (rfapi_vty.c:429)
==7313== by 0x18D0D6: route_vty_out (bgp_route.c:7481)
==7313== by 0x18DD76: bgp_show_table (bgp_route.c:9365)
==7313== by 0x1930C4: bgp_show_table_rd (bgp_route.c:9471)
==7313== by 0x1932A3: bgp_show (bgp_route.c:9510)
==7313== by 0x193E68: show_ip_bgp_json (bgp_route.c:10284)
==7313== by 0x4E6D024: cmd_execute_command_real.isra.2 (command.c:1072)
==7313== by 0x4E6F51E: cmd_execute_command (command.c:1131)
==7313== by 0x4E6F686: cmd_execute (command.c:1285)
==7313== by 0x4EBF9C4: vty_command (vty.c:516)
==7313== by 0x4EBFB9F: vty_execute (vty.c:1285)
==7313== by 0x4EC250F: vtysh_read (vty.c:2119)
==7313==
that is causing the actual crash.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Stephen Worley [Wed, 9 Oct 2019 23:35:46 +0000 (19:35 -0400)]
lib: Don't add/del from name tree if name isnt set
If the name has not been set yet (we were only passed the
ifindex in some cases like with master/slave timings) then
do not add/del it from the ifname rb tree on the vrf struct.
Doing so causes duplicate entries on the tree and infinte loops
can happen when iterating over it.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Stephen Worley [Wed, 9 Oct 2019 20:43:27 +0000 (16:43 -0400)]
lib: Use correct if compare function in tree proto
We were using the incorrect comparison function for the
ifindex-based rb tree. Luckily, we were using the correct one
in RB_GENERATE so I guess that overwrote what was declared in the
prototype?
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Donald Sharp [Tue, 8 Oct 2019 14:36:02 +0000 (10:36 -0400)]
pimd: Fix zlog_warn when we mean debug and vice versa
There are several places in the pim where we are mixing up
zlog_warn w/ zlog_debug and vice versa. If we are protecting
a zlog_warn w/ a debug is it really a warn? If we have an actual
error situation we should also warn about it.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Stephen Worley [Mon, 7 Oct 2019 22:01:21 +0000 (18:01 -0400)]
pbrd: Don't track ipv6 link locals
Don't bother tracking ipv6 link locals to determine if a map
should be installed. Every interface has a route of `fe80::/64`
so its just going to return the arbitrarily first one it finds
when it resolves it and hands it back to us.
Instead, just track the interface we specify along with it.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Quentin Young [Mon, 7 Oct 2019 15:36:39 +0000 (15:36 +0000)]
lib: reject leading 0 in ipv4 decimal quad
inet_pton() is used to parse ipv4 addresses internally, therefore FRR
does not support octal notation for quads. The ipv4 cli token validator
should make sure that str2prefix() can parse tokens it allows, and
str2prefix uses inet_pton, so we have to disallow leading zeros in ipv4
quads.
In short, 1.1.1.01 is no longer valid and must be expressed as 1.1.1.1.
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Donald Sharp [Thu, 3 Oct 2019 13:26:46 +0000 (09:26 -0400)]
doc: Add some additional warnings around Turning off bgp capabilities
FRR supports the ability to turn off the negotation of bgp capabilities.
Provide a few bread crumbs to the operator that it might not be as
useful as they would hope.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
broke the usage of ZEBRA_ROUTE_ALL as a valid redistribution
command. This commit puts it back in. LDP uses ZEBRA_ROUTE_ALL
as an option to say it is interested in all REDISTRIBUTION events.
Fixes: #5072 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Wed, 2 Oct 2019 13:27:04 +0000 (09:27 -0400)]
sharpd: Start infrastructure to allow for redistribution testing
Start the work in sharpd to allow the testing of redistribution
of routes. Namely telling zebra to tell us about redistribution events
via the callback.
Future work here will allow sharpd to specify the redistribution
events it wants and to allow us to track that via counters.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Mon, 30 Sep 2019 12:49:40 +0000 (08:49 -0400)]
tests: Add a topology that shows broken ibgp behavior
In a leaf/spine topology with only IBGP connections, where
the same network is being redistributed at multiple points
in the network ( say a redistribute connected at both leaf and spines )
we end up in a state where zebra gets very confused.
eva# 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 route, r - rejected route
C>* 192.168.1.0/24 is directly connected, tor1-eth0, 00:00:30
C>* 192.168.2.0/24 is directly connected, tor1-eth1, 00:00:30
B 192.168.3.0/24 [200/0] via 192.168.4.2 inactive, 00:00:25
via 192.168.6.2 inactive, 00:00:25
B>* 192.168.4.0/24 [200/0] via 192.168.2.3, tor1-eth1, 00:00:25
* via 192.168.6.2 inactive, 00:00:25
C>* 192.168.5.0/24 is directly connected, tor1-eth2, 00:00:30
B>* 192.168.6.0/24 [200/0] via 192.168.4.2 inactive, 00:00:25
* via 192.168.5.4, tor1-eth2, 00:00:25
Effectively we have ibgp routes recursing through ibgp routes
and there is no metric to discern whom to listen to.
This draft:
https://tools.ietf.org/html/draft-ietf-idr-bgp-optimal-route-reflection-19
appears to address this issue. From looking at both cisco and arista
deployments they are handling this issue by having the route reflector
prefer the localy learned routes over from their clients.
Add this topology, in a broken state, so that when we do fix this issue
it is a simple matter of touching this topology up and re-adding it
to the normal daily builds. I also wanted to add this topology
since it is in a state of `doneness` and I wanted to move onto
my normal day job without having to remember about this test.
This topology is not configured to be run as part of the normal
topotests.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Christian Franke [Mon, 30 Sep 2019 23:53:44 +0000 (01:53 +0200)]
isisd: Fix handling of neighbor circuit id in three way handshake
RFC 5303 states:
If the system ID and Extended Local Circuit ID of the neighboring
system are known (in adjacency three-way state Initializing or
Up), the neighbor's system ID SHALL be reported in the Neighbor
System ID field, and the neighbor's Extended Local Circuit ID
SHALL be reported in the Neighbor Extended Local Circuit ID field.
There is nothing written about only setting the Extended circuit ID of the
adjacency only when we bring the three-way adjacency up.
In fact, we should always update it, to avoid the problem described in #4783.
Fixes: #4783 Signed-off-by: Christian Franke <chris@opensourcerouting.org>
projects / mirror_frr.git / log