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.

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

Create Staticd #2122

Merged
merged 14 commits into from
Aug 8, 2018
Merged

Create Staticd #2122

merged 14 commits into from
Aug 8, 2018

Conversation

donaldsharp
Copy link
Member

Remove the handling of static routes from zebra into it's own daemon

@donaldsharp
Copy link
Member Author

I pushed this up so that I could easily get an idea of what would fail on redhat and to get an idea of what might fail in our topotests.

I know I need to write some documentation and work on some basic static route testing.

@mwinter-osr
Copy link
Member

Assigned to myself to verify the ANVL failures and fix the ANVL scripts to work with the new staticd

@pguibert6WIND
Copy link
Member

pguibert6WIND commented Apr 30, 2018

Hello,
yes some documentation will be welcome to understand why a static daemon.
static, you mean "ip/ipv6 route " will no more be in zebra ?

@mwinter-osr mwinter-osr self-requested a review May 4, 2018 22:52
Copy link
Member

@mwinter-osr mwinter-osr left a comment

Choose a reason for hiding this comment

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

While trying to adopt topotests, I found the following issue:

If the user is still using per-daemon config files and upgrades to this version, then all static routes get lost (as they are stored in zebra.conf and not in staticd.conf)

One possibility to work around this is if there is no staticd.conf and non-integrated configs are used, then staticd could parse (and read the statics) from zebra.conf.
Or maybe there are better choices?

(Topotests has this issue as well as it uses per-daemon configs and not the integrated config)

@mwinter-osr
Copy link
Member

@donaldsharp ANVL (Protocol Compliance) Tests are updated to support staticd

@mwinter-osr mwinter-osr removed their assignment May 6, 2018
Copy link
Member

@rwestphal rwestphal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work! Moving static routes to a separate daemon is certainly the right thing to do. I love how we were able to remove a considerable amount of complexity from zebra, especially in the NHT code. staticd itself looks great but I think we can simplify it even further later (please check #1819 (comment)). For now I think this's already a huge step forward.

I found s few small problems so please check my inline comments.

bool mpls_enabled;

zebra_capabilities_t _caps_p[] = {
ZCAP_NET_RAW, ZCAP_BIND, ZCAP_NET_ADMIN,
Copy link
Member

Choose a reason for hiding this comment

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

I believe staticd doesn't need any of these capabilities.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe you are right. removed.

if (!stable)
return;

zlog_debug("Nexthop num: %d", nh_num);
Copy link
Member

Choose a reason for hiding this comment

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

Unguarded debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

si->nh_valid = !!nh_num;

if (orig != si->nh_valid) {
zlog_debug("Install the matching si");
Copy link
Member

Choose a reason for hiding this comment

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

Unguarded debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

struct zapi_route nhr;
afi_t afi = AFI_IP;

zapi_nexthop_update_decode(zclient->ibuf, &nhr);
Copy link
Member

Choose a reason for hiding this comment

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

Need to check the return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

case STATIC_IPV4_GATEWAY_IFNAME:
case STATIC_IFNAME:
case STATIC_BLACKHOLE:
case STATIC_IPV6_GATEWAY_IFNAME:
Copy link
Member

Choose a reason for hiding this comment

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

I think fully specified static routes (STATIC_IPV4_GATEWAY_IFNAME and STATIC_IPV6_GATEWAY_IFNAME) should also register their nexthops to zebra. Any reason for not doing so?

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, we were not registering NEXTHOP_TYPE_IPV4_IFNAME and NEXTHOP_TYPE_IPV6_IFNAME before. They can have no recursion since we are fully specifying the gateway and ifindex.

memset(&api, 0, sizeof(api));
api.vrf_id = vrf_id;
api.type = ZEBRA_ROUTE_STATIC;
api.safi = SAFI_UNICAST;
Copy link
Member

Choose a reason for hiding this comment

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

Question: shouldn't this be SAFI_MULTICAST for routes configured with ip mroute?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep fixed.

api_nh->gate = si->addr;
break;
case STATIC_IPV4_GATEWAY_IFNAME:
if (si->ifindex == IFINDEX_INTERNAL)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to check if si->nh_valid is true here too (this is related to my other comment about fully-specified static routes).

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, NEXTHOP_TYPE_IPV4_IFINDEX cannot be not valid. They are by definition valid.

@@ -21,812 +21,3 @@
*/
#include <zebra.h>

#include <lib/nexthop.h>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not removing this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can remove.

uint32_t table_id;
char buf[PREFIX_STRLEN];

prefix2str(&p, buf, sizeof(buf));
Copy link
Member

Choose a reason for hiding this comment

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

I think CS will complain because this buffer and other things in this function are being set but not used.

Copy link
Member Author

Choose a reason for hiding this comment

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

correct, this was the start of some code I needed to finish up. Finished up.

static_zebra_nht_register(si, true);

si = rn->info;
static_zebra_route_add(rn, si->vrf_id, true);
Copy link
Member

Choose a reason for hiding this comment

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

On my tests staticd segfaults in this line (si is NULL) if I do this:

debian(config)# ip route 50.0.0.0/8 10.0.1.2
debian(config)# ip route 60.0.0.0/8 50.0.0.1
debian(config)#
root@debian:~#
root@debian:~# ip link add vrf-red type vrf table 10
root@debian:~# ip link set dev vrf-red up
root@debian:~# ip link set dev rt1-eth0 master vrf-red

Could you check what's the problem here?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@donaldsharp
Copy link
Member Author

@pguibert6WIND I will add some documentation to this PR. I thought I had done so. apparently not.

Copy link
Member

@qlyoung qlyoung left a comment

Choose a reason for hiding this comment

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

Mostly just source file license headers need to be changed.

* Copyright (C) 2018 Cumulus Networks, Inc.
* Donald Sharp
*
* FRR is free software; you can redistribute it and/or modify it
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

* Copyright (C) 2018 Cumulus Networks, Inc.
* Donald Sharp
*
* FRR is free software; you can redistribute it and/or modify it
Copy link
Member

Choose a reason for hiding this comment

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

license header

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

* Copyright (C) 2018 Cumulus Networks, Inc.
* Donald Sharp
*
* FRR is free software; you can redistribute it and/or modify it
Copy link
Member

Choose a reason for hiding this comment

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

License header

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed all headers

* under the terms of the GNU General Public License as published by the
* Free Software Foundation; either version 2, or (at your option) any
* later version.
*
* Quagga is distributed in the hope that it will be useful, but
Copy link
Member

Choose a reason for hiding this comment

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

Since we're changing this, wanna replace the whole header?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just removed the files, instead

* Copyright (C) 2018 Cumulus Networks, Inc.
* Donald Sharp
*
* FRR is free software; you can redistribute it and/or modify it
Copy link
Member

Choose a reason for hiding this comment

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

license header

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

"%% Malformed source address\n");
else
zlog_warn(
"%s: Malformed Source address: %s",
Copy link
Member

Choose a reason for hiding this comment

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

Source vs source

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@pguibert6WIND
Copy link
Member

pguibert6WIND commented May 9, 2018

There will be two changes to think of.

  • static daemon will have to be aware on the vrf backend type.
    so that some commands may / may not be available for the daemon
  • documentation will have to be updated to not reference zebra, but staticd

@donaldsharp
Copy link
Member Author

I've already started the work for this communication via the ZEBRA_CAPABILITIES message. Feel free to start adding to it.

@donaldsharp
Copy link
Member Author

I've updated to latest and brought in the changes in zebra that were moved.
I've added documentation
I believe that renato's concern about being able to handle zebra post connection just works given the way staticd holds routes it cannot insert due to insufficient information.

Have I missed anything?

@donaldsharp donaldsharp force-pushed the zebra_nhs branch 2 times, most recently from 7046d38 to 4e4a742 Compare June 21, 2018 10:54
@FRRouting FRRouting deleted a comment from NetDEF-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from LabN-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from NetDEF-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from LabN-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from NetDEF-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from LabN-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from NetDEF-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from NetDEF-CI Jun 21, 2018
@FRRouting FRRouting deleted a comment from LabN-CI Jun 21, 2018
As part of moving the static route handling to it's own daemon
allow zebra to accept static route types from upper level
protocols.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
This is the start of separating out the static
handling code from zebra -> staticd.  This will
help simplify the zebra code and isolate static
route handling to it's own code base.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Add code to allow FRR to properly build and handle the staticd
for some of the more common packaging.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
When the user is not using the integrated config and
has upgraded to a version of FRR with staticd, that
probably means that static routes are stored in zebra.conf.
Take advantage of the backup backup config option and read
those in.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
These are no longer needed so remove.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Existing NEXTHOP_TYPE_IPV4_IFINDEX and NEXTHOP_TYPE_IPV6_IFINDEX
routes allow recursion, but were broken when the route happened
to recursively resolve and the resolution nexthop changed.

This commit fixes this issue.  Please note that this issue was
in pre-move of static route handling to it's own daemon as well.
This was some easy low-hanging fruit, so to speak.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Remove the ip route specific sections from zebra documenation and
create a specific one for the new staticd.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
1) Conform staticd to proper gnu gpl file format.
2) Fix a spelling mistake found.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Allow protocols to specify to zebra that they would like zebra
to use the distance passed down as part of determine sameness for
Route Replace semantics.

This will be used by the static daemon to allow it to have
backup static routes with greater distances.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Modify staticd to allow it to have backup static routes
with higher admin distance.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Move the call to the static_install_route to inside
of the loop, since we have changed the behavior of
how we send down routes to zebra a bit.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
When we read in a backup file, we should save the original
host.config so that we can put it back to the correct original
location after we read in the backup config.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
CI found a couple of warnings that needed to be cleaned up.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
@donaldsharp
Copy link
Member Author

@louberger when I remove that space the script generates a different error. Known issue with it there. I'm consistent with other places in the code for that structure

@LabN-CI
Copy link
Collaborator

LabN-CI commented Jul 29, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2122 51c4ed0
Date 07/29/2018
Start 13:10:52
Finish 13:33:47
Run-Time 22:55
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-07-29-13:10:52.txt
Log autoscript-2018-07-29-13:11:37.log.bz2

For details, please contact louberger

@louberger
Copy link
Member

louberger commented Jul 29, 2018 via email


static int static_vrf_disable(struct vrf *vrf)
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when a VRF is disabled and the ID is reused for another VRF? We're not clearing out the VRF ID that was set in static_vrf_enable(), is it possible to get conflicts here or is it OK to keep the IDs cached?

(Also, the answer to this question should be a code comment here because it'll be hard to understand when someone else looks at this in a year...)

Copy link
Member Author

Choose a reason for hiding this comment

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

well it will never be reused( at least most likely will never be reused as that it's a uint32_t number that is increased monotomically per new interface ), I think your point stands though.

What the vrf code does, as I recall, if we shutdown the vrf we disable routes and when it is turned back on we scan the code base and fixup the nhop vrf id's.( at least that is what I recall programming for static routes 4-5 months back ). All daemons that are vrf aware are probably going to have to be aware of this though.

@donaldsharp
Copy link
Member Author

@louberger I'm not currently interested in fixing that script. We can file an issue and get it addressed though. I'm not sure what the answer is because I think it's valid code either way.

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4663/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.


Warnings Generated during build:

Checkout code: Successful with additional warnings:

<stdin>:207: trailing whitespace.
Starting STATIC 
<stdin>:243: trailing whitespace.
   is ``null0`` then zebra installs a blackhole route.  TABLENO 
<stdin>:247: trailing whitespace.
   allows you to create a leaked route with a nexthop in the specified VRFNAME 
<stdin>:156: new blank line at EOF.
+
<stdin>:325: new blank line at EOF.
+
warning: 5 lines add whitespace errors.
cp: cannot stat './zebra/zebra_static.c': No such file or directory
cp: cannot stat './zebra/zebra_static.h': No such file or directory
Report for static_main.c
===============================================
ERROR: space prohibited before that close parenthesis ')'
#111: FILE: /tmp/f1-31306/static_main.c:111:
+		.privs = &static_privs, )
Report for static_routes.h
===============================================
WARNING: do not add new typedefs
#38: FILE: /tmp/f1-31306/static_routes.h:38:
+typedef enum {
Report for static_vty.c
===============================================
WARNING: unnecessary whitespace before a quoted newline
#467: FILE: /tmp/f1-31306/static_vty.c:467:
+				vty_out(vty, "%% Malformed flag %s \n",
Report for zebra_rnh.c
===============================================
> #740: FILE: /tmp/f2-31306/zebra_rnh.c:740:
59c101
< #718: FILE: /tmp/f1-31306/zebra_rnh.c:718:
Report for zebra_rnh.h
===============================================
> #83: FILE: /tmp/f2-31306/zebra_rnh.h:83:
14c52
< #78: FILE: /tmp/f1-31306/zebra_rnh.h:78:
Report for zebra_vty.c
===============================================
> #2830: FILE: /tmp/f2-31306/zebra_vty.c:2830:
139c182
< #1535: FILE: /tmp/f1-31306/zebra_vty.c:1535:

CLANG Static Analyzer Summary

  • Github Pull Request 2122, comparing to Git base SHA 155d6d4

Fixed warnings:

  • API: Argument with "nonnull" attribute passed null in zebra/zebra_static.c, function static_add_route, line 476

Static Analysis warning summary compared to base:

  • Fixed warnings: 1
  • New warnings: 0

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4663/artifact/shared/static_analysis/index.html

@louberger
Copy link
Member

louberger commented Jul 30, 2018 via email

@eqvinox
Copy link
Contributor

eqvinox commented Aug 8, 2018

The fix (and correct thing to do here) is to make it

FRR_DAEMON_INFO(staticd, STATIC, .vty_port = STATIC_VTY_PORT,
[...]
        .privs = &static_privs,
)

The rationale for this is, both for the , and the ) on a separate line, to have one line each be a functional unit. This is to say that when a patch wants to insert another element, it just adds 1 line before the ). But this only works if you neither have to change the previous line to add the , nor have to add the extra linebreak before the ).

In general, writing lists and struct initializers with a trailing comma and a newline before the closing ) or } is almost always good style.

Other than that: LGTM, please add the linebreak and merge ASAP :)

Just add a linebreak.

Signed-off-by: David Lamparter <equinox@diac24.net>
@eqvinox
Copy link
Contributor

eqvinox commented Aug 8, 2018

Actually went and pushed the fix on your repo (you had "allow maintainers to push changes" ticked :D), let's see if the script still spits an error :)

@LabN-CI
Copy link
Collaborator

LabN-CI commented Aug 8, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/2122 ba86c88
Date 08/08/2018
Start 10:10:54
Finish 10:33:54
Run-Time 23:00
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-08-08-10:10:54.txt
Log autoscript-2018-08-08-10:11:36.log.bz2

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4766/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.


Warnings Generated during build:

Checkout code: Successful with additional warnings:

<stdin>:207: trailing whitespace.
Starting STATIC 
<stdin>:243: trailing whitespace.
   is ``null0`` then zebra installs a blackhole route.  TABLENO 
<stdin>:247: trailing whitespace.
   allows you to create a leaked route with a nexthop in the specified VRFNAME 
<stdin>:156: new blank line at EOF.
+
<stdin>:325: new blank line at EOF.
+
warning: 5 lines add whitespace errors.
cp: cannot stat './zebra/zebra_static.c': No such file or directory
cp: cannot stat './zebra/zebra_static.h': No such file or directory
Report for static_routes.h
===============================================
WARNING: do not add new typedefs
#38: FILE: /tmp/f1-2620/static_routes.h:38:
+typedef enum {
Report for static_vty.c
===============================================
WARNING: unnecessary whitespace before a quoted newline
#467: FILE: /tmp/f1-2620/static_vty.c:467:
+				vty_out(vty, "%% Malformed flag %s \n",
Report for zebra_rnh.c
===============================================
> #740: FILE: /tmp/f2-2620/zebra_rnh.c:740:
59c101
< #718: FILE: /tmp/f1-2620/zebra_rnh.c:718:
Report for zebra_rnh.h
===============================================
> #83: FILE: /tmp/f2-2620/zebra_rnh.h:83:
14c52
< #78: FILE: /tmp/f1-2620/zebra_rnh.h:78:
Report for zebra_vty.c
===============================================
> #2830: FILE: /tmp/f2-2620/zebra_vty.c:2830:
139c182
< #1535: FILE: /tmp/f1-2620/zebra_vty.c:1535:

CLANG Static Analyzer Summary

  • Github Pull Request 2122, comparing to Git base SHA 155d6d4

Fixed warnings:

  • API: Argument with "nonnull" attribute passed null in zebra/zebra_static.c, function static_add_route, line 476

Static Analysis warning summary compared to base:

  • Fixed warnings: 1
  • New warnings: 0

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-4766/artifact/shared/static_analysis/index.html

@mwinter-osr mwinter-osr merged commit 0989048 into FRRouting:master Aug 8, 2018
@donaldsharp donaldsharp deleted the zebra_nhs branch September 13, 2018 14:40
cfra pushed a commit to opensourcerouting/frr that referenced this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants