ctdb-eventscripts: CTDB_NATGW_PUBLIC_* optional on slave-only nodes

Commit 4ee4925d416a86341bd76c11fa99ec9173682a1d forgot about
CTDB_NATGW_SLAVE_ONLY so it introduces an incorrect failure when this
is set, and CTDB_NATGW_PUBLIC_IFACE or CTDB_NATGW_PUBLIC_IP is unset.

Relax the sanity check to see if CTDB_NATGW_SLAVE_ONLY is set.

Update the documentation to explicitly state that
CTDB_NATGW_PUBLIC_IFACE and CTDB_NATGW_PUBLIC_IP are optional and
unused if CTDB_NATGW_SLAVE_ONLY is set.  It would be possible to
insist that CTDB_NATGW_PUBLIC_IFACE and CTDB_NATGW_PUBLIC_IFACE should
be unset in that case.  However, it is more reasonable to allow
consistent configuration across nodes except with some nodes
configured slave-only.

Add tests, update infrastructure and fix a thinko in the stub's
"natgwlist" implementation.

Signed-off-by: Martin Schwenke <martin@meltin.net>
Reviewed-by: Amitay Isaacs <amitay@gmail.com>

Autobuild-User(master): Martin Schwenke <martins@samba.org>
Autobuild-Date(master): Mon Apr 14 06:06:49 CEST 2014 on sn-devel-104

9 files changed, 142 insertions, 17 deletions

diff --git a/ctdb/config/events.d/11.natgw b/ctdb/config/events.d/11.natgw
index b2a219cdeb..f925d4b735 100755
--- a/ctdb/config/events.d/11.natgw
+++ b/ctdb/config/events.d/11.natgw
@@ -18,10 +18,12 @@ natgw_check_config ()
 {
     [ -r "$CTDB_NATGW_NODES" ] || \
 	die "error: CTDB_NATGW_NODES=${CTDB_NATGW_NODES} unreadable"
-    [ -n "$CTDB_NATGW_PUBLIC_IP" ] || \
-	die "Invalid configuration: CTDB_NATGW_PUBLIC_IP not set"
-    [ -n "$CTDB_NATGW_PUBLIC_IFACE" ] || \
-	die "Invalid configuration: CTDB_NATGW_PUBLIC_IFACE not set"
+    if [ "$CTDB_NATGW_SLAVE_ONLY" != "yes" ] ; then
+	[ -n "$CTDB_NATGW_PUBLIC_IP" ] || \
+	    die "Invalid configuration: CTDB_NATGW_PUBLIC_IP not set"
+	[ -n "$CTDB_NATGW_PUBLIC_IFACE" ] || \
+	    die "Invalid configuration: CTDB_NATGW_PUBLIC_IFACE not set"
+    fi
     [ -n "$CTDB_NATGW_PRIVATE_NETWORK" ] || \
 	die "Invalid configuration: CTDB_NATGW_PRIVATE_NETWORK not set"
 
diff --git a/ctdb/doc/ctdbd.conf.5.xml b/ctdb/doc/ctdbd.conf.5.xml
index 188a4df093..98d0924932 100644
--- a/ctdb/doc/ctdbd.conf.5.xml
+++ b/ctdb/doc/ctdbd.conf.5.xml
@@ -537,7 +537,11 @@
 	  <term>CTDB_NATGW_SLAVE_ONLY=yes|no</term>
 	  <listitem>
 	    <para>
-	      When set to "yes" a node can not be a NAT gateway master node.
+	      When set to "yes" a node can not be a NAT gateway master
+	      node.  In this case
+	      <varname>CTDB_NATGW_PUBLIC_IFACE</varname> and
+	      <varname>CTDB_NATGW_PUBLIC_IP</varname> are optional
+	      and unused.
 	    </para>
 	    <para>
 	      Default is no.
diff --git a/ctdb/tests/eventscripts/11.natgw.003.sh b/ctdb/tests/eventscripts/11.natgw.003.sh
new file mode 100755
index 0000000000..cb2358fe2e
--- /dev/null
+++ b/ctdb/tests/eventscripts/11.natgw.003.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+define_test "CTDB_NATGW_PUBLIC_IFACE unset, not slave-only"
+
+setup_ctdb
+setup_ctdb_natgw <<EOF
+192.168.1.21 master
+192.168.1.22
+192.168.1.23
+192.168.1.24
+EOF
+
+CTDB_NATGW_PUBLIC_IFACE=""
+
+required_result 1 "Invalid configuration: CTDB_NATGW_PUBLIC_IFACE not set"
+
+for i in "startup" "ipreallocated" "monitor" ; do
+    simple_test_event "$i"
+done
diff --git a/ctdb/tests/eventscripts/11.natgw.004.sh b/ctdb/tests/eventscripts/11.natgw.004.sh
new file mode 100755
index 0000000000..76f1a54e25
--- /dev/null
+++ b/ctdb/tests/eventscripts/11.natgw.004.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+define_test "CTDB_NATGW_PUBLIC_IP unset, not slave-only"
+
+setup_ctdb
+setup_ctdb_natgw <<EOF
+192.168.1.21 master
+192.168.1.22
+192.168.1.23
+192.168.1.24
+EOF
+
+CTDB_NATGW_PUBLIC_IP=""
+
+required_result 1 "Invalid configuration: CTDB_NATGW_PUBLIC_IP not set"
+
+for i in "startup" "ipreallocated" "monitor" ; do
+    simple_test_event "$i"
+done
diff --git a/ctdb/tests/eventscripts/11.natgw.005.sh b/ctdb/tests/eventscripts/11.natgw.005.sh
new file mode 100755
index 0000000000..94a80c8882
--- /dev/null
+++ b/ctdb/tests/eventscripts/11.natgw.005.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+define_test "Inconsistent test setup: slave-only but current node is master"
+
+setup_ctdb
+setup_ctdb_natgw <<EOF
+192.168.1.21 master
+192.168.1.22
+192.168.1.23
+192.168.1.24
+EOF
+
+CTDB_NATGW_SLAVE_ONLY="yes"
+
+required_result 1 <<EOF
+Inconsistent test configuration - master node is slave-only
+There is no NATGW master node
+EOF
+
+for i in "ipreallocated" "monitor" ; do
+    simple_test_event "$i"
+done
diff --git a/ctdb/tests/eventscripts/11.natgw.041.sh b/ctdb/tests/eventscripts/11.natgw.041.sh
new file mode 100755
index 0000000000..22dd3929bc
--- /dev/null
+++ b/ctdb/tests/eventscripts/11.natgw.041.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+define_test "CTDB_NATGW_SLAVE_ONLY=yes, CTDB_NATGW_PUBLIC_IFACE unset"
+
+setup_ctdb
+setup_ctdb_natgw <<EOF
+192.168.1.21
+192.168.1.22 master
+192.168.1.23
+192.168.1.24
+EOF
+
+CTDB_NATGW_PUBLIC_IFACE=""
+CTDB_NATGW_SLAVE_ONLY="yes"
+
+ok_null
+simple_test_event "ipreallocated"
+
+ok "default via ${FAKE_CTDB_NATGW_MASTER} dev ethXXX  metric 10 "
+simple_test_command ip route show
diff --git a/ctdb/tests/eventscripts/11.natgw.042.sh b/ctdb/tests/eventscripts/11.natgw.042.sh
new file mode 100755
index 0000000000..9019d4a670
--- /dev/null
+++ b/ctdb/tests/eventscripts/11.natgw.042.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+define_test "CTDB_NATGW_SLAVE_ONLY=yes, CTDB_NATGW_PUBLIC_IP unset"
+
+setup_ctdb
+setup_ctdb_natgw <<EOF
+192.168.1.21
+192.168.1.22 master
+192.168.1.23
+192.168.1.24
+EOF
+
+CTDB_NATGW_PUBLIC_IFACE=""
+CTDB_NATGW_PUBLIC_IP=""
+CTDB_NATGW_SLAVE_ONLY="yes"
+
+ok_null
+simple_test_event "ipreallocated"
+
+ok "default via ${FAKE_CTDB_NATGW_MASTER} dev ethXXX  metric 10 "
+simple_test_command ip route show
diff --git a/ctdb/tests/eventscripts/scripts/local.sh b/ctdb/tests/eventscripts/scripts/local.sh
index 4bc2e6c05a..deaab60190 100644
--- a/ctdb/tests/eventscripts/scripts/local.sh
+++ b/ctdb/tests/eventscripts/scripts/local.sh
@@ -535,6 +535,7 @@ setup_ctdb_natgw ()
     export CTDB_NATGW_PUBLIC_IP="10.1.1.121/24"
     export CTDB_NATGW_PUBLIC_IFACE="eth1"
     export CTDB_NATGW_DEFAULT_GATEWAY="10.1.1.254"
+    export CTDB_NATGW_SLAVE_ONLY=""
 }
 
 ok_natgw_master_ip_addr_show ()
diff --git a/ctdb/tests/eventscripts/stubs/ctdb b/ctdb/tests/eventscripts/stubs/ctdb
index 51c17635fc..b8b3e67a42 100755
--- a/ctdb/tests/eventscripts/stubs/ctdb
+++ b/ctdb/tests/eventscripts/stubs/ctdb
@@ -249,21 +249,28 @@ ctdb_natgwlist ()
     [ -r "$CTDB_NATGW_NODES" ] || \
 	die "error: missing CTDB_NATGW_NODES=${CTDB_NATGW_NODES}"
 
-    # Determine the master node
-    _master="-1 0.0.0.0"
+    # Determine if the current node has the (fake) NAT gateway
+    # capability.  This is only used to make sure tests are sane and
+    # don't try to use inconsistent setup.
     if [ -r "$FAKE_CTDB_NATGW_STATE" ] ; then
 	read _state <"$FAKE_CTDB_NATGW_STATE"
-	if [ "$_state" = "on" ] ; then
-	    _pnn=0
-	    while read _ip ; do
-		if [ "$FAKE_CTDB_NATGW_MASTER" = "$_ip" ] ; then
-		    _master="${_pnn} ${_ip}"
-		    break
-		fi
-		_pnn=$(($_pnn + 1))
-	    done <"$CTDB_NATGW_NODES"
-	fi
+    else
+	_state="off"
     fi
+
+    # Determine the master node
+    _master="-1 0.0.0.0"
+    _pnn=0
+    while read _ip ; do
+	if [ "$FAKE_CTDB_NATGW_MASTER" = "$_ip" ] ; then
+	    _master="${_pnn} ${_ip}"
+	    if [ "$_pnn" =  "$FAKE_CTDB_PNN" -a "$_state" = "off" ] ; then
+		die "Inconsistent test configuration - master node is slave-only"
+	    fi
+	    break
+	fi
+	_pnn=$(($_pnn + 1))
+    done <"$CTDB_NATGW_NODES"
     echo "$_master"
 
     # Now print the node information - it is clearer to do this in a