Adding OSPF cost-shifting as a CARP failover mechanism for scenarios requiring rapid failover in HA pairs. #557

quadrinary · 2018-08-23T18:34:38Z

This is an rebased version of Pull Request #413

Code remains the same.

This latest modification to the Quagga package allows users to select the previously existing CARP mode that outright disables the Quagga service or use a hot/warm configuration wherein routers in a HA pair have their OSPF interface costs shifted according to CARP status, allowing for rapid transition from one router to another. Crosslinked in redmine:

Tweak for new output in status_ospfd.php

rbgarga · 2018-08-23T20:08:58Z

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

+			$ospfd_conf['carpmode'] = "quaggadisable";
+			syslog(LOG_NOTICE, "Quagga: CARP failover mode established as 'quaggadisable'");
+		}
+		else {


Move else to same line of }, it's desired style in pfSense

rbgarga · 2018-08-23T20:09:00Z

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

+			$ospfd_conf['carpmode'] = "none";
+			syslog(LOG_NOTICE, "Quagga: CARP failover mode established as 'none'");
+		}
+		write_config( $write_config_only = true );


write_config() expects a string parameter saying the reason of that change to make it easier for users to identify changes.

OK - how would this look, something along the lines of "write_config( "Applied upgrade script" )"?

I had pulled the "write_config( $write_config_only = true )" function from another part of the original plugin and wasn't entirely sure how it was to be used or what it was expecting.

rbgarga · 2018-08-23T20:10:45Z

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

 	exit;
 fi
 EOF;
 	}
+
+	$carp_ospfcost_status_check = "";
+	if ((isset($ospfd_conf['carpmode']) && $ospfd_conf['carpmode'] == "ospfcost") && (isset($ospfd_conf['carpcostvid']) && substr($ospfd_conf['carpcostvid'],0,4) != "none") && (isset($ospfd_conf['carpactivecost']) && isset($ospfd_conf['carpbackupcost'])) && ($ospfd_conf['carpactivecost'] >= 0 && $ospfd_conf['carpactivecost'] <= 65535) && ($ospfd_conf['carpbackupcost'] >= 0 && $ospfd_conf['carpbackupcost'] <= 65535)) {


Please break this really long line, it's hard to read. Use 4 spaces to indent multi-line statements

rbgarga · 2018-08-23T20:13:29Z

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

 		return null;
 	}
+	/* Verify "carpmode" parameters are properly set.*/
+	if ( $ospfd_conf['carpmode'] == "ospfcost" && (!isset($ospfd_conf['carpcostvid']) || substr($ospfd_conf['carpcostvid'],0,4) == "none") || !isset($ospfd_conf['carpactivecost']) || !isset($ospfd_conf['carpbackupcost']) || ($ospfd_conf['carpactivecost'] < 0 || $ospfd_conf['carpactivecost'] > 65535) || ($ospfd_conf['carpbackupcost'] < 0 || $ospfd_conf['carpbackupcost'] > 65535)) {


Break long line

rbgarga · 2018-08-23T20:14:31Z

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

+write_quagga_running_config(bgpd);
+write_quagga_running_config(ospfd);
+write_quagga_running_config(ospf6d);
+write_quagga_running_config(zebra);


Why did you remove quotes here?

Very good question. My production versions haven't had the quotes, so they must've been added in after I submitted the original code 18 months ago that included the "write_quagga_running_config()" function. I'll do some testing on my end to ensure it doesn't impact functionality.

jim-p

Aside from the issues already pointed out by @rbgarga I just have one small note.

jim-p · 2018-08-24T12:37:54Z

net/pfSense-pkg-Quagga_OSPF/files/usr/local/www/status_ospfd.php

@@ -78,9 +78,9 @@ function doCmdT($title, $command) {
 defCmdT("Quagga BGP IPv6 Routes", "{$control_script} bgp6 route");
 defCmdT("Quagga BGP Neighbors", "{$control_script} bgp neighbor");
 defCmdT("Quagga BGP Summary", "{$control_script} bgp sum");
-defCmdT("Quagga ospfd.conf", "/bin/cat {$pkg_homedir}/ospfd.conf");


Showing the configuration file and the running configuration are two completely different and useful things. The running config is what was interpreted by quagga, so it may differ from the configuration file, and that can aid in tracking down problems with a configuration. You can add the commands to show the running configuration, but do not remove the commands to show the configuration files.

I understand your reasoning here.

My logic behind making it a pure "show running-config" vs. the file that was loaded when the daemon started is that we wanted to see at a glance if an OSPF cost had been set dynamically by viewing the running configuration directly.

With that in mind, I can do as you suggested and show both.

quadrinary · 2018-08-28T13:29:47Z

I've made and tested the requested changes and normalized the multi-line functions as they had been done in different manners over time.

quadrinary · 2018-09-17T19:26:21Z

Two things I wish to note:

I scripted an "upgrade" path for users that had previously used the "traditional" CARP failover method. It checks to see if CARP was previously being monitored and sets that as the desired operational mode at the time of upgrade.
For testing, I'll suggest a few things.

When selecting your active/standby OSPF costs, I generally give the standby unit lesser values in each field. For example, on my primary units, I set the OSPF "active" cost to 9 and the "backup" cost to 19000. On the standby unit, I set OSPF "active" cost to 5 and the "backup" cost to 15000.

That way, if there is a CARP event, the LSA containing the updated cost values from the standby unit will always be more preferable when it is taking over; if both units had the same "active" cost, then there is a chance that connected routers may attempt equal cost load balancing, which does not play well with Quagga, and is anyway not what you want when you've just experienced a CARP event.

Likewise, when you fail to "backup" state, you want your costs to be more extreme on the primary unit so that it takes itself out of the OSPF equation if the secondary unit is taking a bit to get its LSAs out.

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

jim-p · 2018-09-26T20:37:18Z

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

 		log_error("Quagga: No assistant generated config for OSPF, but found raw config for one or more daemon");
 	} else {
 		log_error("Quagga OSPFd: No config data found.");
 		return;
 	}

 	if (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['ospfd'])
-	    && !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['ospfd'])) {
+		&& !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['ospfd'])) {


These should be indented with four spaces

jim-p · 2018-09-26T20:37:23Z

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

@@ -266,7 +266,7 @@ function quagga_ospfd_install_conf() {

 	/* Make zebra config */
 	if (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['zebra'])
-	    && !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['zebra'])) {
+		&& !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['zebra'])) {


These should be indented with four spaces

jim-p · 2018-09-26T20:37:27Z

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

@@ -291,11 +291,10 @@ function quagga_ospfd_install_conf() {

 	/* Make bgpd config, add password and logging */
 	if (isset($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])
-	    && !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])) {
+		&& !empty($config['installedpackages']['quaggaospfdraw']['config'][0]['bgpd'])) {


These should be indented with four spaces

jim-p · 2018-09-26T20:38:03Z

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

 		if ($bgpddelmd5file != "") {
 			$fd = fopen("{$quagga_config_base}/bgpddelmd5pw.conf", "w");
 			fwrite($fd, $bgpddelmd5file);
 			fclose($fd);
 		}
 	}
-        /* Make ospf6 config */
+		/* Make ospf6 config */


Why the extra tab here?

jim-p · 2018-09-26T20:38:38Z

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']));
 	} else {
-        	$ospf6dconffile = "";
+			$ospf6dconffile = "";


Looks like maybe one tab too many here as well

jim-p · 2018-09-26T20:39:08Z

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

+
+	$carp_ospfcost_status_check = "";
+	if ((isset($ospfd_conf['carpmode']) && $ospfd_conf['carpmode'] == "ospfcost")
+		&& (isset($ospfd_conf['carpcostvid']) && substr($ospfd_conf['carpcostvid'],0,4) != "none")


All of these conditions should be indented with four spaces

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

jim-p · 2018-09-26T20:42:48Z

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

@@ -514,24 +558,86 @@ function quagga_ospfd_plugin_carp($pluginparams) {
 	if (is_array($config['installedpackages']['quaggaospfd']['config'])) {
 		$ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];


On 2.4.4 and later, this syntax can be dangerous with PHP 7.2 if the user doesn't have any settings.

Check out init_config_arr() for a good way around this, for example, instead of the is_array() test, use:

init_config_arr(array('installedpackages', 'quaggaospfd', 'config', 0)); $ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];

Do that any time there is an assignment by reference to a multi-level hash like this.

(I know this wasn't part of your commits, but since you're working on it anyhow...)

I need to think about this a bit more since init_config_arr() did not exist on earlier versions of pfsense, and would immediately break the version we are running. Which production version of pfSense was the first to receive this function? I can setup a conditional to use is_array() in older versions.

2.4.4, but 2.4.4 is also the first to run PHP 7.2. You can't just do one is_array() test you have to test and create each level below it, or PHP 7.2 will barf if it doesn't exist. 2.4.4 is the current supported branch so it is the priority and all new development must be supported there over anything else.

Based on what I'm observing, Init_config_arr() was only introduced in 2.4.4 (config revision 18.8 based on this site https://www.netgate.com/docs/pfsense/releases/versions-of-pfsense-and-freebsd.html ). I'll include a parameter to check the config revision version and if it's 18.8 or greater, it'll use init_config_arr(). If less, it'll use is_array().

Side note - is relying on $config['version'] a suitable solution to determine the pfSense version in play or is there a better way?

You won't be able to install the new version of the package on versions older than 2.4.4, so a version test is unnecessary in the copy of the package that gets committed.

Aside from that, the best way to check the version is to look at $g['product_version'] and then use one of the version compare functions like version_compare_string().

Got it. I could see where this particular use of is_array() could cause a problem as it's called during any CARP event, whereas the rest are assuming a config is either being built or has been built. That said, I'll test it with a new install and verify init_config_arr() works as expected in this situation.

I just tested with a new install of 2.4.4, configured a CARP address, and added the existing Quagga version, which relies on is_array(). I added logging to the conditions in question to see what happens. I performed a number of CARP tests and no PHP crashes occurred in the cases where there was no Quagga config and in the case where there was a Quagga config.

In looking at init_config_arr(), I'm trying to understand how it should be applied in this case, since the purpose of the if-else statement is to check if a config actually exists. If we have the init_config_arr() function create an array, then won't it artificially pass the test every time? Or would we replace the if-else with the array initialization and let it go to the subsequent tests?

In that kind of case, the is_array() test may be OK but you still need to initialize the array before assigning by reference. It may be safe in certain cases and not in others, depends on the state of the user configuration. Just something to look out for. We've been fixing a lot of similar cases in the base and packages ever since the switch to PHP 7.2.

Is this the optimal place to initialize the array then? Should it be initialized somewhere else besides the CARP function call?

As long as it gets initialized before assigning by reference, it doesn't matter much. You could initialize first and then change the is_array() check to an !empty() check.

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

Replaced tabs with spaces as requested.

Removed debug logging from CARP tracking.

rbgarga · 2019-12-17T12:23:00Z

@quadrinary sorry for the long delay on this PR. Could you please rebase your fork so we can review and test it?

rbgarga · 2020-02-13T11:43:13Z

Superseded by #769

3.1.0 (2022-05-18) * Introduce basic support for OpenSSL version 3 (#492) * Update regex in grep to be POSIX compliant (#556) * Introduce status reporting tools (#555 & #557) * Display certificates using UTF8 (#551) * Allow certificates to be created with fixed date offset (#550) * Add 'verify' to verify certificate against CA (#549) * Add PKCS#12 alias 'friendlyName' (#544) * Disallow use of '--vars=FILE init-pki' (#566) * Support multiple IP-Addresses in SAN (#564) * Add option '--renew-days=NN', custom renew grace period (#557) * Add 'nopass' option to the 'export-pkcs' functions (#411) * Add support for 'busybox' (#543) * Add option '--tmp-dir=DIR' to declare Temp-dir (Commit f503a22) 3.0.9 (2022-05-17) * Upgrade OpenSSL from 1.1.0j to 1.1.1o (#405, #407) - We are buliding this ourselves now. * Fix --version so it uses EASYRSA_OPENSSL (#416) * Use openssl rand instead of non-POSIX mktemp (#478) * Fix paths with spaces (#443) * Correct OpenSSL version from Homebrew on macOs (#416) * Fix revoking a renewed certificate (Original PR #394) Follow-up commit: ef22701878bb10df567d60f2ac50dce52a82c9ee * Introduce 'show-crl' (d1993892178c5219f4a38d50db3b53d1a972b36c) * Support Windows-Git 'version of bash' (#533) * Disallow use of single quote (') in vars file, Warning (#530) * Creating a CA uses x509-types/ca and COMMON (#526) * Prefer 'PKI/vars' over all other locations (#528) * Introduce 'init-pki soft' option (#197) * Warnings are no longer silenced by --batch (#523) * Improve packaging options (#510) * Update regex for POSIX compliance (#556) * Correct date format for Darwin/BSD (#559)

quadrinary added 13 commits August 23, 2018 14:16

Update Makefile

36f1fd2

Update status_ospfd.php

f0175f4

Update quagga_ospfd.inc

8e25ace

Update quagga_ospfd.xml

2143d84

Adding OSPF Cost Shifting for HA failover

288d521

Adding OSPF Cost Shifting for HA failover

afd93fb

Update quagga_ospfd.inc

8ebf68b

Adding OSPF Cost Shifting for HA failover

f09973a

Adding OSPF Cost Shifting for HA failover

f016281

Adding OSPF Cost Shifting for HA failover

46193f2

Adding OSPF Cost Shifting for HA failover

3a09d46

Adding OSPF Cost Shifting for HA failover

2ff32ad

Adding OSPF Cost Shifting for HA failover

3615d7f

Tweak for new output in status_ospfd.php

rbgarga requested changes Aug 23, 2018

View reviewed changes

rbgarga requested a review from jim-p August 23, 2018 20:14

jim-p requested changes Aug 24, 2018

View reviewed changes

quadrinary added 2 commits August 28, 2018 09:26

Update quagga_ospfd.inc

846636d

Update status_ospfd.php

2cd2b36

sbeaver-netgate added the Pending review/testing label Aug 30, 2018

rbgarga requested review from jim-p and rbgarga September 26, 2018 11:01

jim-p requested changes Sep 26, 2018

View reviewed changes

quadrinary added 2 commits September 26, 2018 21:40

Update quagga_ospfd.inc

83e1d68

Replaced tabs with spaces as requested.

Update quagga_ospfd.inc

18e7818

Removed debug logging from CARP tracking.

rbgarga added the Conflicts label May 30, 2019

vktg mentioned this pull request Feb 13, 2020

Quagga OSPF cost-shifting as a CARP failover. Issue #8181 #769

Merged

rbgarga closed this Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding OSPF cost-shifting as a CARP failover mechanism for scenarios requiring rapid failover in HA pairs. #557

Adding OSPF cost-shifting as a CARP failover mechanism for scenarios requiring rapid failover in HA pairs. #557

quadrinary commented Aug 23, 2018

rbgarga Aug 23, 2018

quadrinary Aug 27, 2018

rbgarga Aug 23, 2018

quadrinary Aug 27, 2018

rbgarga Aug 23, 2018

quadrinary Aug 27, 2018

rbgarga Aug 23, 2018

quadrinary Aug 27, 2018

rbgarga Aug 23, 2018

quadrinary Aug 27, 2018

jim-p left a comment

jim-p Aug 24, 2018

quadrinary Aug 27, 2018

quadrinary commented Aug 28, 2018

quadrinary commented Sep 17, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

jim-p Sep 26, 2018

quadrinary Sep 27, 2018

jim-p Sep 27, 2018

quadrinary Sep 27, 2018

jim-p Sep 27, 2018

quadrinary Sep 27, 2018

quadrinary Sep 27, 2018

jim-p Sep 27, 2018

quadrinary Sep 28, 2018

jim-p Sep 28, 2018

rbgarga commented Dec 17, 2019

rbgarga commented Feb 13, 2020

		@@ -514,24 +558,86 @@ function quagga_ospfd_plugin_carp($pluginparams) {
		if (is_array($config['installedpackages']['quaggaospfd']['config'])) {
		$ospfd_conf = &$config['installedpackages']['quaggaospfd']['config'][0];

Adding OSPF cost-shifting as a CARP failover mechanism for scenarios requiring rapid failover in HA pairs. #557

Adding OSPF cost-shifting as a CARP failover mechanism for scenarios requiring rapid failover in HA pairs. #557

Conversation

quadrinary commented Aug 23, 2018

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

jim-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quadrinary commented Aug 28, 2018

quadrinary commented Sep 17, 2018

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

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

rbgarga commented Dec 17, 2019

rbgarga commented Feb 13, 2020