Enable BGP and OSPFv3 #356

fiskn · 2017-05-16T07:35:27Z

Allows raw config to be entered to enable the use of BGP and OSPFv3 and provides required support to control the daemons.

For users who require both BGP and OSPF this is the only way of accomplishing this. Also some people who are using Quagga elsewhere in their infrastructure, means that they have only one type of BGP config to manage.

Based on work from this pull request
https://github.com/pfsense/pfsense-packages/pull/1258/files

jim-p · 2017-05-16T12:40:14Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

@@ -76,8 +76,8 @@ function quagga_ospfd_install_conf() {
 	$noaccept = "";

 	// generate ospfd.conf based on the assistant
-	if (is_array($config['installedpackages']['quaggaospfd']['config'])) {
-		$ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];
+	if (is_array($config['installedpackages']['quaggaospfdraw']['config'])) {


Why are these two lines changed? It is trying to load the non-raw config here into $ospfd_conf which expects it to be an array, not raw configuration text. This will break running a normal (non-raw) configuration.

jim-p · 2017-05-16T12:40:33Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

-	if (is_array($config['installedpackages']['quaggaospfd']['config'])) {
-		$ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];
+	if (is_array($config['installedpackages']['quaggaospfdraw']['config'])) {
+		$ospfd_conf = &$config['installedpackages']['quaggaospfdraw']['config'][0];


See comment for above line.

jim-p · 2017-05-16T12:59:04Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

@@ -312,14 +337,24 @@ if [ -e /var/run/quagga/ospfd.pid ]; then
 	/bin/pkill -F /var/run/quagga/ospfd.pid
 	/bin/rm -f /var/run/quagga/ospfd.pid
 fi
+if [ -e /var/run/quagga/bgpd.pid ]; then
+	kill -9 `cat /var/run/quagga/bgpd.pid`


You should use pkill for this, look 4 lines up from here for an example. Also, commands need to use their full paths (see the same example above)

jim-p · 2017-05-16T12:59:48Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

@@ -312,14 +337,24 @@ if [ -e /var/run/quagga/ospfd.pid ]; then
 	/bin/pkill -F /var/run/quagga/ospfd.pid
 	/bin/rm -f /var/run/quagga/ospfd.pid
 fi
+if [ -e /var/run/quagga/bgpd.pid ]; then
+	kill -9 `cat /var/run/quagga/bgpd.pid`
+	rm -f /var/run/quagga/bgpd.pid


Need to use the full path to rm

jim-p · 2017-05-16T13:01:22Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

 EOF;
 	$rc_file_start = <<<EOF
+/bin/mkdir -p {$quagga_config_base}
+/bin/mkdir -p /var/etc/quagga


This line and the line above it effectively do the same thing, but neither are necessary as the PHP code handles it when writing out the configuration.

jim-p · 2017-05-16T13:16:32Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

 /usr/local/sbin/zebra -d -f {$quagga_config_base}/zebra.conf
 /usr/local/sbin/ospfd -d -f {$quagga_config_base}/ospfd.conf
+/usr/local/sbin/ospf6d -d -f {$quagga_config_base}/ospf6d.conf


Why start this unconditionally? What if its configuration is empty? It should not start if there is no configuration. At a minimum, test that the file exists and has non-zero size.

jim-p · 2017-05-16T13:16:37Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

 /usr/local/sbin/zebra -d -f {$quagga_config_base}/zebra.conf
 /usr/local/sbin/ospfd -d -f {$quagga_config_base}/ospfd.conf
+/usr/local/sbin/ospf6d -d -f {$quagga_config_base}/ospf6d.conf
+/usr/local/sbin/bgpd -d -f {$quagga_config_base}/bgpd.conf


Why start this unconditionally? What if its configuration is empty? It should not start if there is no configuration. At a minimum, test that the file exists and has non-zero size.

jim-p · 2017-05-16T13:17:30Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

 {$carp_ip_status_check}
+/usr/bin/touch {$quagga_config_base}/zebra.conf


What is the purpose of all these touch commands? If the pkg code writes out a valid configuration, it will be present and the touch will not help anything. If the file is not there, the daemon shouldn't try to start. (Same for this and the three lines after)

jim-p · 2017-05-16T13:19:07Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd_raw.xml

+                        <description>
+                                <![CDATA[
+                                Note: Once you click "Save" below, the assistant (in the "Global Settings" and "Interface Settings" tabs above) will be overridden 
+with w$


Looks like a copy/paste word wrap issue here - should be on the previous line and some text appears to be missing.

jim-p · 2017-05-16T13:19:36Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd_raw.xml

+		<field>
+			<fielddescr>bgpd.conf</fielddescr>
+			<fieldname>bgpd</fieldname>
+			<description>Note: Once you click "Save" below, the assistant (in the "Global Settings" and "Interface Settings" tabs above) will be overridden with whatever you type here. To get back the assisted config save this form below once with both empty input fields.</description>


Should probably use CDATA around this as the others do.

fiskn · 2017-05-16T13:33:01Z

Thanks Jim, I will review and update

fiskn · 2017-05-16T15:40:20Z

I have addressed all your comments including starting the daemons, which first checks that the config file is bigger than 0 bytes.

jim-p · 2017-05-16T16:54:08Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

 	}
+	else 


You should keep the braces here. See https://doc.pfsense.org/index.php/Developer_Style_Guide#Indent_style

jim-p · 2017-05-16T16:54:27Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

+	    // if there is a raw config specified in the config.xml use that instead of the assisted config
+	    $ospf6dconffile = str_replace("\r","",base64_decode($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d']));
+	}
+	else


Need to fix the braces here, too: https://doc.pfsense.org/index.php/Developer_Style_Guide#Indent_style

fiskn · 2017-05-16T18:46:29Z

Sorry about the braces, I corrected them

jim-p · 2017-05-16T19:03:07Z

The braces are still not quite right, you need braces around the body of the else statements as well, even if they are only one line.

rbgarga · 2017-05-16T20:19:23Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

@@ -78,6 +78,10 @@ function quagga_ospfd_install_conf() {
 	// generate ospfd.conf based on the assistant
 	if (is_array($config['installedpackages']['quaggaospfd']['config'])) {
 		$ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];
+	} elseif (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['ospfd'])  ||
+		  isset($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d']) ||
+	 	  isset($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])) {


Please fix indent. Multiple lines statements should use 4 spaces on last indent level

Can I just clarify this please. I wasn't sure if the indentation by 4 spaces overrides the requirement that the conditionals align? Due to elseif 4 spaces won't make them align.

As defined in Developer style guide [1], if a conditional statement must span multiple lines it should use 4 spaces indent:

if (foo) { ... } elseif ($foo1 && $foo2 && $foo3 && $foo4 && $foo5 && $foo6 && $foo7 && foo8 && foo9) { ... }

[1] https://doc.pfsense.org/index.php/Developer_Style_Guide

rbgarga · 2017-05-16T20:19:44Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

+	if (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])
+	    && !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])) {
+	    // if there is a raw config specified in the config.xml use that instead of the assisted config
+	    $bgpdconffile = str_replace("\r","",base64_decode($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd']));


Fix indent. 2 lines above should use TABs

rbgarga · 2017-05-16T20:19:56Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

+	    // if there is a raw config specified in the config.xml use that instead of the assisted config
+	    $bgpdconffile = str_replace("\r","",base64_decode($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd']));
+	} else {
+	    $bgpdconffile = "";


rbgarga · 2017-05-16T20:20:07Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

+	if (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d'])
+	    && !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d'])) {
+	    // if there is a raw config specified in the config.xml use that instead of the assisted config
+	    $ospf6dconffile = str_replace("\r","",base64_decode($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d']));


rbgarga · 2017-05-16T20:20:15Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

+	    // if there is a raw config specified in the config.xml use that instead of the assisted config
+	    $ospf6dconffile = str_replace("\r","",base64_decode($config['installedpackages']['quaggaospfdraw']['config'][0]['ospf6d']));
+	} else {
+            $ospf6dconffile = "";


rbgarga · 2017-05-16T20:21:59Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.inc

@@ -348,7 +403,9 @@ EOF;
 	// Ensure files have correct permissions
 	mwexec("/bin/chmod a+rx /usr/local/etc/rc.d/quagga.sh");
 	mwexec("/bin/chmod u+rw,go-rw {$quagga_config_base}/ospfd.conf");
+        mwexec("/bin/chmod u+rw,go-rw {$quagga_config_base}/ospf6d.conf");


Indent using TAB

rbgarga · 2017-05-16T20:22:15Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd.xml

+                <rcfile>quagga.sh</rcfile>
+                <executable>ospf6d</executable>
+                <description>OSPF6 routing daemon</description>
+        </service>


Indent using TAB

rbgarga · 2017-05-16T20:22:23Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd_interfaces.xml

+                <name>Quagga OSPF6d</name>
+                <rcfile>quagga.sh</rcfile>
+                <executable>ospf6d</executable>
+        </service>


Indent using TAB

rbgarga · 2017-05-16T20:22:47Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd_raw.xml

+                <rcfile>quagga.sh</rcfile>
+                <executable>ospf6d</executable>
+        </service>
+


Indent using TAB and please don't add empty lines on XML

rbgarga · 2017-05-16T20:23:07Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/pkg/quagga_ospfd_raw.xml

+			<encoding>base64</encoding>
+			<rows>30</rows>
+			<cols>65</cols>
+		</field>


Indent using TAB and remove empty lines

fiskn · 2017-05-17T11:31:08Z

I have corrected the indentations, would you like me to squash the commits?

rbgarga

After you fix it please squash commits

rbgarga · 2017-05-17T13:11:25Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/bin/quaggactl

+                ;;
+
+        esac ;;
+


This block is still indented using spaces, it must use hard tabs

fiskn · 2017-05-18T15:13:46Z

Commits have been squashed, is there anything else that needs to be done?

quadrinary · 2017-05-23T18:30:47Z

Hi all - I've been running my own version of the plugin for a few months now that integrated BGP in a similar manner. In addition, I added a feature to the raw config page that would load any changes made via "vtysh" that allows you to compare the running vs saved configs. I find this extremely useful and would be interested in seeing it integrated into the main branch. How can I contribute my work to this?

Furthermore, I solved/implemented a fix to the issue of authentication for BGP so that peers can communicate via the specified md5 password. This is also implemented via my raw config page.

Thanks,
quadrinary

quagga_ospfd_1.3.inc.txt
quagga_ospfd_raw_v1.3.xml.txt
quagga_ospfd_v1.3.xml.txt
quaggactl_v1.3.txt
status_ospfd_v1.3.php.txt

While here, add two patches: 1. Fix application startup which was broken by a dependency update (fixed in master); 2. Fix boolean facts graph (pfsense#356).

Recent changes since MooseFS 3.0.112: * MooseFS 3.0.113-1 (2020-05-04) - (master) removed unnecessary debug syslog messages - (check) increased usleep tolerance due to some operating systems (issue #351) - (master) changed condition that decides if master should wait for more chunks during I/O - (mount) delayed setting channel for fuse notifications (very rare segfault in libfuse2 during init) - (client+master) changed conditions for ancestor test in getattr (issue #350) - (cs) added parameter for number of chunks to be send in single register packet - (cs) fixed reporting damaged chunks in testing function (issue #352) - (cs) removed some unnecessary damaged chunk notifications - (client) changed open test in setattr (related to issue #350) - (master) fixed handling truncate for open files (related to issue #350) - (master) added uid mapping in setfacl function - (master+client) added support for atomic truncate with open - (master+client) fixed keep cache conditions - (cs) introduced official label format defined in chunkserver configuration - (nbd) added readonly mode and locking - (cs) fixed condition in choosing disks for internal rebalance - (mount) added workaround in access for a bug in FreeBSD kernel (issue #354) - (cs) fixed master reconnection conditions (reload usually shouldn't cause reconnection) - (freebsd) fixed FreeBSD port (makeports.sh) - (master) changed disk removal detection algorithm (issue #356) - (cs) fixed calculating size limits - (cs) added handling inode limits in local filesystems on chunkservers (issue #358) - (master) added topology grouping when new chunks are about to be created (prefer closer servers) - (master) added more error messages to bgsaver - (cs) added changing subfolder during internal rebalance (related to issue #326) - (master+cs) added optional logging of long function execution times - (all) fixes of small errors found by static code analysers - (mount) turn off dentry invalidator for Linux kernels >= 4.19 (related to issue #357) - (master) changed condition that checks timestamp in changelogs (less prone to small clock differences) - (daemons) added time refresh function (for future use) - (cgi+cli) changed mark for removal state name UNKNOWN->PENDING (related to issue #359) - (client) decreased max IDLE time in writer module from 1s to 0.1s - (client) update inode in dentry invalidator - (tools) fixed mfscopyeattr, mfsseteattr and mfs*arch tools - (master+client) added support for new eattrs: immutable, appendonly, undeletable (setting new undeletable extra attribute appropriately can help with issues like #357) - (client) silenced "kern.proc.filedesc" syslog messages on FreeBSD (issue #360) PR: 246416 Submitted by: MooseFS FreeBSD Team <freebsd@moosefs.pro> (maintainer)

netgate-git-updates added the CLA label May 16, 2017

rbgarga requested a review from jim-p May 16, 2017 10:49

jim-p requested changes May 16, 2017

View reviewed changes

jim-p approved these changes May 16, 2017

View reviewed changes

jim-p requested a review from rbgarga May 16, 2017 20:01

rbgarga requested changes May 16, 2017

View reviewed changes

rbgarga added the Changes requested label May 16, 2017

rbgarga requested changes May 17, 2017

View reviewed changes

Allow Quagga to run bgpd

7e573fe

fiskn force-pushed the devel branch from 7c4dcc9 to 7e573fe Compare May 17, 2017 13:26

rbgarga removed the Changes requested label May 23, 2017

rbgarga approved these changes May 23, 2017

View reviewed changes

netgate-git-updates merged commit 7e573fe into pfsense:devel May 23, 2017

netgate-git-updates pushed a commit that referenced this pull request May 23, 2017

Merge pull request #356 from fiskn/devel

29cc0d7

SysError956 pushed a commit to SysError956/FreeBSD-ports that referenced this pull request Jun 4, 2017

Update to 0.2.2

a1ede04

While here, add two patches: 1. Fix application startup which was broken by a dependency update (fixed in master); 2. Fix boolean facts graph (pfsense#356).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable BGP and OSPFv3 #356

Enable BGP and OSPFv3 #356

fiskn commented May 16, 2017 •

edited

Loading

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

fiskn commented May 16, 2017

fiskn commented May 16, 2017

jim-p May 16, 2017

jim-p May 16, 2017

fiskn commented May 16, 2017

jim-p commented May 16, 2017

rbgarga May 16, 2017

fiskn May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

rbgarga May 16, 2017

fiskn commented May 17, 2017

rbgarga left a comment

rbgarga May 17, 2017

fiskn commented May 18, 2017

quadrinary commented May 23, 2017 •

edited

Loading

		{$carp_ip_status_check}
		/usr/bin/touch {$quagga_config_base}/zebra.conf

Enable BGP and OSPFv3 #356

Enable BGP and OSPFv3 #356

Conversation

fiskn commented May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiskn commented May 16, 2017

fiskn commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiskn commented May 16, 2017

jim-p commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiskn commented May 17, 2017

rbgarga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiskn commented May 18, 2017

quadrinary commented May 23, 2017 • edited Loading

fiskn commented May 16, 2017 •

edited

Loading

quadrinary commented May 23, 2017 •

edited

Loading