diff mbox

NAT: reload BSCs config dynamically

Message ID 1459778794-15778-1-git-send-email-msuraev@sysmocom.de
State Superseded
Headers show

Commit Message

Max April 4, 2016, 2:06 p.m. UTC
From: Max <msuraev@sysmocom.de>

Add vty tests for BSC configuration reloading.
Load BSCs configuration on bscs-config-file command:
* remove all runtime configured BSC not in the config file
* close connections to all BSC with updated token value

Fixes: OS#1670
Sponsored-by: On-Waves ehf
---
 openbsc/include/openbsc/bsc_nat.h      |   5 ++
 openbsc/src/osmo-bsc_nat/bsc_nat.c     |   4 +-
 openbsc/src/osmo-bsc_nat/bsc_nat_vty.c |  35 +++++++-
 openbsc/tests/vty_test_runner.py       | 145 ++++++++++++++++++++++++++++++++-
 4 files changed, 185 insertions(+), 4 deletions(-)

Comments

Holger Freyther April 5, 2016, 9:08 p.m. UTC | #1
> On 04 Apr 2016, at 16:06, msuraev@sysmocom.de wrote:
> 
> 
> @@ -1321,8 +1321,8 @@ static int ipaccess_bsc_read_cb(struct osmo_fd *bfd)
> 			     bsc->cfg ? bsc->cfg->nr : -1);
> 		else
> 			LOGP(DNAT, LOGL_ERROR,
> -			     "Stream error on BSC Nr: %d. Failed to parse ip access message: %d\n",
> -			     bsc->cfg ? bsc->cfg->nr : -1, ret);
> +			     "Stream error on BSC Nr: %d. Failed to parse ip access message: %d (%s)\n",
> +			     bsc->cfg ? bsc->cfg->nr : -1, ret, strerror(-ret));

good! but separate patch. And have a look at where ret is coming from that you need to invert it.



> 
> 	bsc_replace_string(_nat, &_nat->include_file, argv[0]);
> +
> +	llist_for_each_entry_safe(con1, con2, &_nat->bsc_connections,
> +				  list_entry) {
> +		if (con1->cfg)
> +			if (con1->cfg->token_updated || con1->cfg->remove)
> +				bsc_close_connection(con1);
> +	}

In terms of style the following is preferred as it is reducing the level of indentation and makes you see what the code is about (instead of shifting it to the right)

		if (!con1->cfg)
			continue;
		if (!con->cfg->token_updated && !con1->cfg->remove)
			continue;

		bsc_close_connection(con1);


resulting assembly should not be any different and the ultimate action is seen.


> +
> +	llist_for_each_entry_safe(cf1, cf2, &_nat->bsc_configs, entry) {
> +		if (cf1->remove) {
> +			bsc_config_free(cf1);
> +			_nat->num_bsc--;

that one is a "nasty" topic

1.) this should be within  bsc_config_free
2.) 

bsc_config_alloc(..) {
	conf->nr = nat->num_bsc;

and the code assumes that conf->nr is unique so if we do num_bsc-- and then num_bsc++ again we might re-use an existing id and break the assumption..

What I propose (and in a separate patch)

* Remove the requirement to assign BSCs in order. I thought I had lifted it a long time ago (but I did not)

* Change bsc_config_alloc to include the nr to be assigned

* change cfg_bsc_mcd to first search the given BSC and then create one..



holger
diff mbox

Patch

diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h
index ab1c590..c664d24 100644
--- a/openbsc/include/openbsc/bsc_nat.h
+++ b/openbsc/include/openbsc/bsc_nat.h
@@ -35,6 +35,7 @@ 
 #include <osmocom/gsm/protocol/gsm_04_08.h>
 
 #include <regex.h>
+#include <stdbool.h>
 
 #define DIR_BSC 1
 #define DIR_MSC 2
@@ -164,6 +165,10 @@  struct bsc_config {
 	/* audio handling */
 	int max_endpoints;
 
+	/* used internally for reload handling */
+	bool remove;
+	bool token_updated;
+
 	/* backpointer */
 	struct bsc_nat *nat;
 
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c
index 7dcacfd..9bbb253 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c
@@ -1321,8 +1321,8 @@  static int ipaccess_bsc_read_cb(struct osmo_fd *bfd)
 			     bsc->cfg ? bsc->cfg->nr : -1);
 		else
 			LOGP(DNAT, LOGL_ERROR,
-			     "Stream error on BSC Nr: %d. Failed to parse ip access message: %d\n",
-			     bsc->cfg ? bsc->cfg->nr : -1, ret);
+			     "Stream error on BSC Nr: %d. Failed to parse ip access message: %d (%s)\n",
+			     bsc->cfg ? bsc->cfg->nr : -1, ret, strerror(-ret));
 
 		bsc_close_connection(bsc);
 		return -1;
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
index 8def3ed..45e408c 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
@@ -39,6 +39,7 @@ 
 #include <osmocom/sccp/sccp.h>
 
 #include <stdlib.h>
+#include <stdbool.h>
 
 static struct bsc_nat *_nat;
 
@@ -512,15 +513,40 @@  DEFUN(cfg_nat_include,
       "Set the filename of the BSC configuration to include.\n"
       "The filename to be included.")
 {
+	int rc;
+	struct bsc_config *cf1, *cf2;
+	struct bsc_connection *con1, *con2;
 	const char *path = bsc_nat_resolve_path(_nat, _nat->include_base,
 						argv[0]);
-	int rc = vty_read_config_file(path, NULL);
+
+	llist_for_each_entry_safe(cf1, cf2, &_nat->bsc_configs, entry) {
+		cf1->remove = true;
+		cf1->token_updated = false;
+	}
+
+	rc = vty_read_config_file(path, NULL);
 	if (rc < 0) {
 		vty_out(vty, "Failed to parse the config file %s: %s%s", path,
 			strerror(-rc), VTY_NEWLINE);
 		return CMD_WARNING;
 	}
+
 	bsc_replace_string(_nat, &_nat->include_file, argv[0]);
+
+	llist_for_each_entry_safe(con1, con2, &_nat->bsc_connections,
+				  list_entry) {
+		if (con1->cfg)
+			if (con1->cfg->token_updated || con1->cfg->remove)
+				bsc_close_connection(con1);
+	}
+
+	llist_for_each_entry_safe(cf1, cf2, &_nat->bsc_configs, entry) {
+		if (cf1->remove) {
+			bsc_config_free(cf1);
+			_nat->num_bsc--;
+		}
+	}
+
 	return CMD_SUCCESS;
 }
 
@@ -843,6 +869,7 @@  DEFUN(cfg_bsc, cfg_bsc_cmd, "bsc BSC_NR",
 	if (!bsc)
 		return CMD_WARNING;
 
+	bsc->remove = false;
 	vty->index = bsc;
 	vty->node = NAT_BSC_NODE;
 
@@ -855,6 +882,12 @@  DEFUN(cfg_bsc_token, cfg_bsc_token_cmd, "token TOKEN",
 {
 	struct bsc_config *conf = vty->index;
 
+	if (strncmp(conf->token, argv[0], 128) != 0) {
+		vty_out(vty, "updated token: %s -> %s%s", conf->token, argv[0],
+			VTY_NEWLINE);
+		conf->token_updated = true;
+	}
+
 	bsc_replace_string(conf, &conf->token, argv[0]);
 	return CMD_SUCCESS;
 }
diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py
index 8db0825..bd2cefd 100644
--- a/openbsc/tests/vty_test_runner.py
+++ b/openbsc/tests/vty_test_runner.py
@@ -584,12 +584,61 @@  class TestVTYBSC(TestVTYGenericBSC):
 class TestVTYNAT(TestVTYGenericBSC):
 
     def vty_command(self):
-        return ["./src/osmo-bsc_nat/osmo-bsc_nat", "-c",
+        return ["./src/osmo-bsc_nat/osmo-bsc_nat", "-l", "127.0.0.1", "-c",
                 "doc/examples/osmo-bsc_nat/osmo-bsc_nat.cfg"]
 
     def vty_app(self):
         return (4244, "src/osmo-bsc_nat/osmo-bsc_nat",  "OsmoBSCNAT", "nat")
 
+    def testBSCreload(self):
+        # use separate ip to avoid interference with other tests
+        ip = "127.0.0.4"
+        self.vty.enable()
+        bscs1 = self.vty.command("show bsc-config")
+        nat_bsc_reload(self)
+        bscs2 = self.vty.command("show bsc-config")
+        # check that multiple calls to bscs-config-file give the same result
+        self.assertEquals(bscs1, bscs2)
+
+        # add new bsc
+        self.vty.command("configure terminal")
+        self.vty.command("nat")
+        self.vty.command("bsc 2")
+        self.vty.command("token key")
+        self.vty.command("location_area_code 666")
+        self.vty.command("end")
+
+        # update bsc token
+        self.vty.command("configure terminal")
+        self.vty.command("nat")
+        self.vty.command("bsc 1")
+        self.vty.command("token xyu")
+        self.vty.command("end")
+
+        nat_msc_ip(self, ip)
+        msc = nat_msc_test(self, ip)
+        b0 = nat_bsc_sock_test(0, "lol")
+        b1 = nat_bsc_sock_test(1, "xyu")
+        b2 = nat_bsc_sock_test(2, "key")
+
+        self.assertEquals("3 BSCs configured", self.vty.command("show nat bsc"))
+        self.assertTrue(3 == nat_bsc_num_con(self))
+        self.assertEquals("MSC is connected: 1", self.vty.command("show msc connection"))
+
+        nat_bsc_reload(self)
+        bscs2 = self.vty.command("show bsc-config")
+        # check that the reset to initial config succeeded
+        self.assertEquals(bscs1, bscs2)
+
+        self.assertEquals("2 BSCs configured", self.vty.command("show nat bsc"))
+        self.assertTrue(1 == nat_bsc_num_con(self))
+        rem = self.vty.command("show bsc connections").split(' ')
+        # remaining connection is for BSC0
+        self.assertEquals('0', rem[2])
+        # remaining connection is authorized
+        self.assertEquals('1', rem[4])
+        self.assertEquals("MSC is connected: 1", self.vty.command("show msc connection"))
+
     def testVtyTree(self):
         self.vty.enable()
         self.assertTrue(self.vty.verify('configure terminal', ['']))
@@ -971,6 +1020,100 @@  def add_nat_test(suite, workdir):
     test = unittest.TestLoader().loadTestsFromTestCase(TestVTYNAT)
     suite.addTest(test)
 
+def nat_bsc_reload(x):
+    x.vty.command("configure terminal")
+    x.vty.command("nat")
+    x.vty.command("bscs-config-file bscs.config")
+    x.vty.command("end")
+
+def nat_msc_ip(x, ip):
+    x.vty.command("configure terminal")
+    x.vty.command("nat")
+    x.vty.command("msc ip " + ip)
+    x.vty.command("end")
+
+def data2str(d):
+    return "".join("{:02x}".format(ord(c)) for c in d)
+
+def nat_msc_test(x, ip, verbose = False):
+    msc = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    msc.settimeout(32)
+    msc.bind((ip, 5000))
+    msc.listen(5)
+    if (verbose):
+        print "MSC is ready at " + ip
+    while "MSC is connected: 0" == x.vty.command("show msc connection"):
+        conn, addr = msc.accept()
+        if (verbose):
+            print "MSC got connection from ", addr
+    return conn
+
+def ipa_send_pong(x, verbose = False):
+    if (verbose):
+        print "\tBSC -> NAT: PONG!"
+    x.send("\x00\x01\xfe\x01")
+
+def ipa_send_ping(x, verbose = False):
+    if (verbose):
+        print "\tBSC -> NAT: PING?"
+    x.send("\x00\x01\xfe\x00")
+
+def ipa_send_ack(x, verbose = False):
+    if (verbose):
+            print "\tBSC -> NAT: IPA ID ACK"
+    x.send("\x00\x01\xfe\x06")
+
+def ipa_handle_small(x, verbose = False):
+    s = data2str(x.recv(4))
+    if "0001fe00" == s:
+        if (verbose):
+            print "\tBSC <- NAT: PING?"
+        ipa_send_pong(x, verbose)
+    elif "0001fe06" == s:
+        if (verbose):
+            print "\tBSC <- NAT: IPA ID ACK"
+        ipa_send_ack(x, verbose)
+    elif "0001fe00" == s:
+        if (verbose):
+            print "\tBSC <- NAT: PONG!"
+    else:
+        if (verbose):
+            print "\tBSC <- NAT: ", s
+
+def ipa_send_reset(x, verbose = False):
+    if (verbose):
+        print "\tBSC -> NAT: RESET"
+    x.send("\x00\x12\xfd\x09\x00\x03\x05\x07\x02\x42\xfe\x02\x42\xfe\x06\x00\x04\x30\x04\x01\x20")
+
+def ipa_send_resp(x, tk, verbose = False):
+    if (verbose):
+        print "\tBSC -> NAT: IPA ID RESP"
+    x.send("\x00\x07\xfe\x05\x00\x04\x01" + tk)
+
+def ipa_handle_resp(x, tk, verbose = False):
+    s = data2str(x.recv(38))
+    if "0023fe040108010701020103010401050101010011" in s:
+         ipa_send_resp(x, tk, verbose)
+    else:
+        if (verbose):
+            print "\tBSC <- NAT: ", s
+
+def nat_bsc_num_con(x):
+    return len(x.vty.command("show bsc connections").split('\n'))
+
+def nat_bsc_sock_test(nr, tk, verbose = False):
+    bsc = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    bsc.bind(('127.0.0.1' + str(nr), 0))
+    bsc.connect(('127.0.0.1', 5000))
+    if (verbose):
+        print "BSC%d " %nr
+        print "\tconnected to %s:%d" % bsc.getpeername()
+    ipa_handle_small(bsc, verbose)
+    ipa_handle_resp(bsc, tk, verbose)
+    bsc.recv(27) # MGCP msg
+    ipa_handle_small(bsc, verbose)
+    return bsc
+
 def add_bsc_test(suite, workdir):
     if not os.path.isfile(os.path.join(workdir, "src/osmo-bsc/osmo-bsc")):
         print("Skipping the BSC test")