There are lib debugs being set but never show up in
`show debug` commands because there was no way to show
that they were being used. Add a bit of infrastructure
to allow this and then use it for `debug route-map`
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Although VTY_GET_CONTEXT can return a failed value, it will
never happen in pbrd because of how context work. In
any event add some code to make coverity happy
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
PBR rules are installed as match, action rules in most dataplanes. This
requires the action to be resolved via a GW. And the GW to be subsequently
resolved to {SMAC, DMAC}.
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Firstly, *keep no change* for `hash_get()` with NULL
`alloc_func`.
Only focus on cases with non-NULL `alloc_func` of
`hash_get()`.
Since `hash_get()` with non-NULL `alloc_func` parameter
shall not fail, just ignore the returned value of it.
The returned value must not be NULL.
So in this case, remove the unnecessary checking NULL
or not for the returned value and add `void` in front
of it.
Importantly, also *keep no change* for the two cases with
non-NULL `alloc_func` -
1) Use `assert(<returned_data> == <searching_data>)` to
ensure it is a created node, not a found node.
Refer to `isis_vertex_queue_insert()` of isisd, there
are many examples of this case in isid.
2) Use `<returned_data> != <searching_data>` to judge it
is a found node, then free <searching_data>.
Refer to `aspath_intern()` of bgpd, there are many
examples of this case in bgpd.
Here, <returned_data> is the returned value from `hash_get()`,
and <searching_data> is the data, which is to be put into
hash table.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Currently the nexthop tracking code is only sending to the requestor
what it was requested to match against. When the nexthop tracking
code was simplified to not need an import check and a nexthop check
in b8210849b8 for bgpd. It was not
noticed that a longer prefix could match but it would be seen
as a match because FRR was not sending up both the resolved
route prefix and the route FRR was asked to match against.
This code change causes the nexthop tracking code to pass
back up the matched requested route (so that the calling
protocol can figure out which one it is being told about )
as well as the actual prefix that was matched to.
Fixes: #10766
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
When adding a nhg to a route map, make sure to specify the `family`
of the rm by looking at the contents of the nhg. Installation in the
kernel (for DSCP rules in particular) relies on this being specified in
the netlink message.
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
Signed-off-by: Stephen Worley <sworley@nvidia.com>
VRF name should not be printed in the config since 574445ec. The update
was done for NB config output but I missed it for regular vty output.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Currently, it is possible to rename the default VRF either by passing
`-o` option to zebra or by creating a file in `/var/run/netns` and
binding it to `/proc/self/ns/net`.
In both cases, only zebra knows about the rename and other daemons learn
about it only after they connect to zebra. This is a problem, because
daemons may read their config before they connect to zebra. To handle
this rename after the config is read, we have some special code in every
single daemon, which is not very bad but not desirable in my opinion.
But things are getting worse when we need to handle this in northbound
layer as we have to manually rewrite the config nodes. This approach is
already hacky, but still works as every daemon handles its own NB
structures. But it is completely incompatible with the central
management daemon architecture we are aiming for, as mgmtd doesn't even
have a connection with zebra to learn from it. And it shouldn't have it,
because operational state changes should never affect configuration.
To solve the problem and simplify the code, I propose to expand the `-o`
option to all daemons. By using the startup option, we let daemons know
about the rename before they read their configs so we don't need any
special code to deal with it. There's an easy way to pass the option to
all daemons by using `frr_global_options` variable.
Unfortunately, the second way of renaming by creating a file in
`/var/run/netns` is incompatible with the new mgmtd architecture.
Theoretically, we could force daemons to read their configs only after
they connect to zebra, but it means adding even more code to handle a
very specific use-case. And anyway this won't work for mgmtd as it
doesn't have a connection with zebra. So I had to remove this option.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Since f60a1188 we store a pointer to the VRF in the interface structure.
There's no need anymore to store a separate vrf_id field.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
This removes a giant `switch { }` block from lib/zclient.c and
harmonizes all zclient callback function types to be the same (some had
a subset of the args, some had a void return, now they all have
ZAPI_CALLBACK_ARGS and int return.)
Apart from getting rid of the giant switch, this is a minor security
benefit since the function pointers are now in a `const` array, so they
can't be overwritten by e.g. heap overflows for code execution anymore.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
With netns VRF backend, we may have multiple interfaces with the same
name. Currently, the function is not deterministic in this case as it
uses the first interface that it finds in the list. Be more restrictive
and ask the user to provide the VRF name.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
There is a possibility that the same line can be matched as a command in
some node and its parent node. In this case, when reading the config,
this line is always executed as a command of the child node.
For example, with the following config:
```
router ospf
network 193.168.0.0/16 area 0
!
mpls ldp
discovery hello interval 111
!
```
Line `mpls ldp` is processed as command `mpls ldp-sync` inside the
`router ospf` node. This leads to a complete loss of `mpls ldp` node
configuration.
To eliminate this issue and all possible similar issues, let's print an
explicit "exit" at the end of every node config.
This commit also changes indentation for a couple of existing exit
commands so that all existing commands are on the same level as their
corresponding node-entering commands.
Fixes#9206.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The only difference in daemons' interface node definition is the config
write function. No need to define the node in every daemon, just pass
the callback as an argument to a library function and define the node
there.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Add `match src-port (1-65535)` and `match dst-port (1-65535)`
commands to allow pbr to pass these values down to zebra.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Start the inclusion of src_prt and dst_prt in
the internal data structures. At this point
we do not do anything with the data other
than pass down what we have stored in pbrd.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
If we have the following configuration:
```
vrf red
smth
exit-vrf
!
interface red vrf red
smth
```
And we delete the VRF using "no vrf red" command, we end up with:
```
interface red
smth
```
Interface config is preserved but moved to the default VRF.
This is not an expected behavior. We should remove the interface config
when the VRF is deleted.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When handling the addition of a nhg which would saturate the range of
tables PBR is configured to install into, handle this *before* a
possible call to pbr_nhgc_alloc during hash query
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
Represent installed ("allocated") NHG tables with a hash keyed by table
ID. Replaces a pre-allocated array of booleans that implemented this
functionality before.
+ PBR table range > 65535 is fixed (was OOB access before :))
+ Pre-compute next available ID to save time when only checking
if all tables are allocated
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
Most of these are many, many years out of date. All of them vary
randomly in quality. They show up by default in packages where they
aren't really useful now that we use integrated config. Remove them.
The useful ones have been moved to the docs.
Signed-off-by: Quentin Young <qlyoung@nvidia.com>
Back when I put this together in 2015, ISO C11 was still reasonably new
and we couldn't require it just yet. Without ISO C11, there is no
"good" way (only bad hacks) to require a semicolon after a macro that
ends with a function definition. And if you added one anyway, you'd get
"spurious semicolon" warnings on some compilers...
With C11, `_Static_assert()` at the end of a macro will make it so that
the semicolon is properly required, consumed, and not warned about.
Consistently requiring semicolons after "file-level" macros matches
Linux kernel coding style and helps some editors against mis-syntax'ing
these macros.
Signed-off-by: David Lamparter <equinox@diac24.net>
Neither tabs nor newlines are acceptable in syslog messages. They also
break line-based parsing of file logs.
Signed-off-by: David Lamparter <equinox@diac24.net>
Only handle an interface update in the nexthop tracking code
if the nexthop in question was set with an interface to point
out of. If the nexthop is GW only, the interface update could
be unrelated but have overlapping address space. Let that be
handled elsewhere.
Ex)
```
5.5.5.0/30 dev dummyDoof proto kernel scope link src 5.5.5.1
5.5.5.0/24 dev goofDummy proto kernel scope link src 5.5.5.1
[root@alfred frr-2]# ip ro show table 10000
default via 5.5.5.2 dev dummyDoof proto pbr metric 20
[root@alfred frr-2]# ip link set goofDummy down
[root@alfred frr-2]# ip ro show table 10000
[root@alfred frr-2]# ip link set goofDummy up
[root@alfred frr-2]# ip ro show table 10000
```
Signed-off-by: Stephen Worley <sworley@nvidia.com>
I am not even sure what the goal of this code was in any
way shape fashion or form. But since it's pbr_nht.c
I as the original author should know... But I don't.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
the pnhi data structure can receive either a interface or a
nhr data structure. Ensure that we don't crash.
CID -> 1500586
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Disallow mismatching of ipv4/ipv6 matching in src/dst.
Doesn't make a lot of sense to allow this based on how
IP Headers work. The kernel does not allow it at all
obviously.
Signed-off-by: Stephen Worley <sworley@nvidia.com>
When an interface goes up/down we need to pay attention to this
in PBR. In the past we were relying *only* on the nht events
but this is not sufficient for cases where an interface is flapping
up and down. If this is happening it could be happening fast enough
that zebra is not sending nht events because they are consolidated
into a single event from it's perspective and that is the right thing
to do. This commit will allow us to back out commit:
0aaa722883
As that commit introduced extra processing in zebra that is actually
causing issues in other places. The problem that commit was trying
to solve should have always been handled in pbrd instead of making
zebra do work that is unnatural to it's actual flow.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The `enum zclient_send_status` enum needs to be extended
throughout the code base to use the new states and
to fix up places where we tested against the return
value being non zero.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Issue:
The bgp routes learnt from peers which are not installed in kernel are
advertised to peers. This can cause routers to send traffic to these
destinations only to get dropped. The fix is to provide a configurable
option "bgp suppress-fib-pending". When the option is enabled, bgp will
advertise routes only if it these are successfully installed in kernel.
Fix (Part1) :
* Added message ZEBRA_ROUTE_NOTIFY_REQUEST used by client to request
FIB install status for routes
* Added AFI/SAFI to ZAPI messages
* Modified the functions zapi_route_notify_decode(), zsend_route_notify_owner()
and route_notify_internal() to include AFI, SAFI as parameters
Signed-off-by: kssoman <somanks@gmail.com>
On shutdown pbr was leaking the ifp->info ( struct pbr_interface *)
pointer.
Add some code to notice we are being shutdown and cleanup the memory
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
This should never happen; no need to debug guard it and it's not a
warning, if this isn't working then NHT is not working at all.
Signed-off-by: Quentin Young <qlyoung@nvidia.com>
Use a bool as the return val for pbr_send_pbr_map() to make
the code a bit more readable. Dont expect there to be need
for values other than true or false anyway.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Properly cleanup the pbr interface data if nothing actually
gets sent to zebra, since we will never get the callback
notification from zapi to issue final deletion.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Add a return val so caller can know if something was actually sent to
zebra here. Some things need to be cleanued up by the caller
if we arent getting a callback from zapi.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
If we are experiencing an interface that is bouncing
very fast and the last operation that we experienced
was a ifdown we will send rule deletions associated
with that interface. If we have not received notification
that hte rule was removed *but* we immiedately get another
ifup notification when we go to install the rule we
are deciding that it's not ready to send down again,
as that we still think it is installed.
Force the rule installation when we have a interface up
event.
Ticket: CM-31042
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
When installing rules pass by the interface name across
zapi.
This is being changed because we have a situation where
if you quickly create/destroy ephermeal interfaces under
linux the upper level protocol may be trying to add
a rule for a interface that does not quite exist
at the moment. Since ip rules actually want the
interface name ( to handle just this sort of situation )
convert over to passing the interface name and storing
it and using it in zebra.
Ticket: CM-31042
Signed-off-by: Stephen Worley <sworley@nvidia.com>
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Whenever libyang loads a module that contains a leafref, it will
also implicitly load the module of the referring node if it's
not loaded already. That makes sense as otherwise it wouldn't be
possible to validate the leafref value correctly.
The problem is that loading a module implicitly violates the
assumption of the northbound layer that all loaded modules
are implemented (i.e. they have a northbound node associated
to each schema node). This means that loading a module that
isn't implemented can lead to crashes as the "priv" pointer
of schema nodes is no longer guaranteed to be valid. To fix this
problem, add a few null checks to ignore data nodes associated
to non-implemented modules.
The side effect of this change is harmless. If a daemon receives
configuration it doesn't support (e.g. BFD peers on staticd),
that configuration will be stored but otherwise ignored. This can
only happen when using a northbound client like gRPC, as the CLI
will never send to a daemon a command it doesn't support. This
minor problem should go away in the long run as FRR migrates to
a centralized management model, at which point the YANG-modeled
configuration of all daemons will be maintained in a single place.
Finally, update some daemons to stop implementing YANG modules
they don't need to (i.e. revert 1b741a01c and a74b47f5).
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
PR #6376 introduced a VRF leafref in the frr-interface YANG module.
That change exposed a bug in the northbound layer that is causing
pbrd to crash under certain circumstances. Even though pbrd wasn't
converted to the new northbound model yet, make it implement the
frr-vrf module in order to work around this problem. This is a
temporary fix until a better solution is available.
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
Given a received nexthop update, only send down an update to the
relevant nexthop group. Avoid sending down superfluous updates
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
The pnhc->nexthop was a pointer copy. Causing issues
with the ability to move pointers around for the
different pnhc since the pnhc mirrored the nexthop
caches. When we received a vrf change if we shared
pointers it was impossible to know if we had
already updated the code.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
We had multiple pnhc cache entries with the same nexthop
pointer. This causes some large amount of confusion.
Fixup the code to handle this situation better.
Ticket: CM-31044
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
PBR needs the ability to allow ephermeal interfaces( bonds,
vrfs, dummy, bridges, etc ) to be destroyed and then
recreated and at the same time keep track of them and
rebuild state as appropriate when we get a change.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
The nexthop_group_write_nexthop_simple function outputs the
interface name, because we've stored the ifindex. The problem
is that there are ephermeal interfaces in linux that can be
destroyed/recreated. Allow us to keep that data and do something
a bit smarter to allow show run's and other show commands to continue
to work when the interface is deleted.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Currently when a vrf is deleted than added back in PBR was
not going through and touching up all the data structures
that needed to be massaged to allow it to start working again.
This includes:
a) Search through the nexthop groups to find any nexthop
that references the old nexthop id and set it right again.
b) Search through the nexthop cache for nht and reset
those nexthops to the right vrf as well as re-register
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Matching by dscp may now also be specified by its standard codepoint
(provided it has one), such as `cf0` or `af11`.
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
Extend PBR maps to discriminate by Differentiated Services Code Point and / or
Explicit Congestion Notification fields. These fields are used in the IP header
for classifying network traffic.
0 1 2 3 4 5 6 7
+-----+-----+-----+-----+-----+-----+-----+-----+
| DS FIELD, DSCP | ECN FIELD |
+-----+-----+-----+-----+-----+-----+-----+-----+
DSCP: differentiated services codepoint
ECN: Explicit Congestion Notification
Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>
Signed-off-by: Saurav Kumar Paul <saurav@cumulusnetworks.com>
Cleanup the marking of a nhc as installed/removed based on table
route installation.
We were not even handling the removal state at all.
We saw some timing issues with the routes being installed/removed
multiple times and then never resending the pbr map due to bad states
on the nhgc.
Dont worry about checking if its already marked installed before
scheduling the policy walk. We have a check in `pbr_send_map()`
to ensure we dont try to resend a map sequence already installed.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Withdraw route from RIB if we detect the NHG is not valid
anymore. We were seeing an issue where we would leave a route
in zebra's RIB if it was recursive despite it being removed
from the kernel due to an interface going down.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
When specifying an interface in a pbr-map `set nexthop ..` command
be a bit more lenient about the interface.
a) If the interface does not exist bail on the command
(this is the same)
b) If the interface exists but is in a different vrf
than specified use the vrf it is actually in.
(this is new behavior)
Ticket: CM-30187
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Fix a number of library and daemon issues so that daemons can
call frr_fini() during normal termination. Without this,
temporary logging files are left behind in /var/tmp/frr/.
Signed-off-by: Mark Stapp <mjs@voltanet.io>
Don't crash when trying to `show running-config` because of missing
filter northbound integration.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Revise new `show pbr` keys to be consistent with existing
json in other daemons
target->nexthop
id->tableId (where relevant)
isValid->valid
isInstalled->installed
Signed-off-by: Wesley Coakley <wcoakley@cumulusnetworks.com>
