Merge ~ballot/charm-mongodb/+git/charm-mongodb:LP#1930552 into charm-mongodb:master

[Benjamin Allot]

Add host and port in the replicaset check LP#1930552

The check considers that the replicaset name is the mongodb host to
check.

That is not a valid assumption, so let's parse the configuration file
and determine what it should be

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

Unmerged commits

74f45e3... by Benjamin Allot on 2022-01-27

Add host and port in the replicaset check LP#1930552

The check considers that the replicaset name is the mongodb host to
check.

That is not a valid assumption, so let's parse the configuration file
and determine what it should be.

d142401... by Benjamin Allot on 2022-01-27

Use pytest instead of unittests

Because everyone deserves better looking tests output and debugging
interface.

diff --git a/files/nrpe-external-master/check_replica_sets.py b/files/nrpe-external-master/check_replica_sets.py
index f6d8feb..ec36dea 100755
--- a/files/nrpe-external-master/check_replica_sets.py
+++ b/files/nrpe-external-master/check_replica_sets.py
@@ -23,6 +23,9 @@ REPL_STATES_OK = ["PRIMARY", "SECONDARY", "ARBITER"]
 # REPL_STATES_WARN = ["STARTUP", "STARTUP2", "ROLLBACK", "RECOVERING"]
 REPL_STATES_CRIT = ["UNKNOWN", "DOWN", "REMOVED"]
 
+DEFAULT_BIND = "localhost"
+DEFAULT_PORT = "27017"
+
 
 def get_cmd_result(cmd):
     try:
@@ -36,6 +39,9 @@ def get_replica_set_name():
     # mongo2 file format the key is 'replSet'
     # mongo34 yaml file format the key is 'replSetName'
     # this will match both
+    replset = False
+    host = DEFAULT_BIND
+    port = DEFAULT_PORT
 
     with open("/etc/mongodb.conf", "r") as mongo_file:
         data = mongo_file.read()
@@ -45,19 +51,34 @@ def get_replica_set_name():
             config = None
 
         if isinstance(config, dict):
-            if "replication" not in config:
-                return False
-            if "replSetName" not in config["replication"]:
-                return False
-            return config["replication"]["replSetName"]
-
-        for line in data.splitlines():
-            if line.strip().startswith("replSet") is False:
-                continue
-            parts = line.split("=")
-            return parts[1].strip()
-
-    return False
+            # Host and port can be avoided considering default value can be used
+            try:
+                host = config["net"]["bindIp"]
+            except KeyError:
+                pass
+            try:
+                port = config["net"]["port"]
+            except KeyError:
+                pass
+            # Fail hard if replicaset name is absent
+            try:
+                replset = config["replication"]["replSetName"]
+            except KeyError:
+                replset = False
+        else:  # Old configuration format
+            for line in data.splitlines():
+                if line.strip().startswith("replSet"):
+                    replset = line.split("=")[1].strip()
+                if line.strip().startswith("bind_ip"):
+                    host = line.split("=")[1].strip()
+                if line.strip().startswith("port"):
+                    port = line.split("=")[1].strip()
+
+    # Translate 0.0.0.0 to localhost for connection purpose
+    if host == "0.0.0.0":
+        host = "localhost"
+
+    return host, port, replset
 
 
 def get_replica_status():
@@ -74,13 +95,19 @@ def get_replica_status():
     #     ]
     # }
 
-    name = get_replica_set_name()
-    if not name:
-        print("CRITIAL: unable to determine the replica set name")
+    host, port, name = get_replica_set_name()
+    if not name or not host or not port:
+        print("CRITICAL: unable to determine the host, port and replica set name")
         sys.exit(NAGIOS_CRIT)
 
     mongo_res = get_cmd_result(
-        ["/usr/bin/mongo", name, "--quiet", "--eval", "JSON.stringify(rs.status())"]
+        [
+            "/usr/bin/mongo",
+            "{}:{}".format(host, port),
+            "--quiet",
+            "--eval",
+            "JSON.stringify(rs.status('{}'))".format(name),
+        ]
     )
 
     try:
diff --git a/tests/unit/test_check_replica_sets.py b/tests/unit/test_check_replica_sets.py
index 5eb9b6e..65c8def 100644
--- a/tests/unit/test_check_replica_sets.py
+++ b/tests/unit/test_check_replica_sets.py
@@ -44,8 +44,30 @@ class TestCheckReplicaSets(unittest.TestCase):
         with mock.patch(
             "builtins.open", mock.mock_open(read_data=sample_yaml)
         ) as mock_file:
-            res = get_replica_set_name()
+            host, port, res = get_replica_set_name()
             mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "27017")
+            self.assertEqual(res, "test")
+
+    def test_get_replica_set_name_yaml_bind_and_port(self):
+        """Test mongodb.conf file parsing replSetName success."""
+        sample = {}
+        sample["replication"] = {}
+        sample["replication"]["replSetName"] = "test"
+        sample["net"] = {
+            "bindIp": "0.0.0.0",
+            "port": "12345",
+        }
+        sample_yaml = yaml.dump(sample)
+
+        with mock.patch(
+            "builtins.open", mock.mock_open(read_data=sample_yaml)
+        ) as mock_file:
+            host, port, res = get_replica_set_name()
+            mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "12345")
             self.assertEqual(res, "test")
 
     def test_get_replica_set_name_yaml_no_replsetname(self):
@@ -57,8 +79,10 @@ class TestCheckReplicaSets(unittest.TestCase):
         with mock.patch(
             "builtins.open", mock.mock_open(read_data=sample_yaml)
         ) as mock_file:
-            res = get_replica_set_name()
+            host, port, res = get_replica_set_name()
             mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "27017")
             self.assertEqual(res, False)
 
     def test_get_replica_set_name_yaml_no_replication(self):
@@ -70,8 +94,10 @@ class TestCheckReplicaSets(unittest.TestCase):
         with mock.patch(
             "builtins.open", mock.mock_open(read_data=sample_yaml)
         ) as mock_file:
-            res = get_replica_set_name()
+            host, port, res = get_replica_set_name()
             mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "27017")
             self.assertEqual(res, False)
 
     def test_get_replica_set_name_ini_true(self):
@@ -90,8 +116,10 @@ class TestCheckReplicaSets(unittest.TestCase):
         """
 
         with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
-            res = get_replica_set_name()
+            host, port, res = get_replica_set_name()
             mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "27017")
             self.assertEqual(res, "test")
 
     def test_get_replica_set_name_ini_no_replset(self):
@@ -109,17 +137,42 @@ class TestCheckReplicaSets(unittest.TestCase):
         """
 
         with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
-            res = get_replica_set_name()
+            host, port, res = get_replica_set_name()
             mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "27017")
             self.assertEqual(res, False)
 
+    def test_get_replica_set_name_ini_bind0_and_nodefault_port(self):
+        """Test mongodb.conf file missing replSet in ini format."""
+        sample = """
+            # mongodb.conf
+            dbpath=/var/lib/mongodb
+            ipv6=true
+            logpath=/var/log/mongodb/mongodb.log
+            logappend=true
+            bind_ip = 0.0.0.0
+            port = 12345
+            journal=true
+            diaglog = 0
+            rest = true
+            replSet = test
+        """
+
+        with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
+            host, port, res = get_replica_set_name()
+            mock_file.assert_called_with("/etc/mongodb.conf", "r")
+            self.assertEqual(host, "localhost")
+            self.assertEqual(port, "12345")
+            self.assertEqual(res, "test")
+
     @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
     @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
     def test_get_replica_status_ok(
         self, mock_get_cmd_result, mock_get_replica_set_name
     ):
         """Test replica status in known good states gives OK."""
-        mock_get_replica_set_name.return_value = "test"
+        mock_get_replica_set_name.return_value = "localhost", "27017", "test"
         mock_get_cmd_result.return_value = """{
             "set" : "myset",
             "members" : [
@@ -149,7 +202,7 @@ class TestCheckReplicaSets(unittest.TestCase):
         self, mock_get_cmd_result, mock_get_replica_set_name
     ):
         """Test replica status in RECOVERING state gives WARN."""
-        mock_get_replica_set_name.return_value = "test"
+        mock_get_replica_set_name.return_value = "localhost", "27017", "test"
         mock_get_cmd_result.return_value = """{
             "set" : "myset",
             "members" : [
@@ -185,7 +238,7 @@ class TestCheckReplicaSets(unittest.TestCase):
         self, mock_get_cmd_result, mock_get_replica_set_name
     ):
         """Test replica status in DOWN state gives CRIT."""
-        mock_get_replica_set_name.return_value = "test"
+        mock_get_replica_set_name.return_value = "localhost", "27017", "test"
         mock_get_cmd_result.return_value = """{
             "set" : "myset",
             "members" : [
@@ -218,9 +271,9 @@ class TestCheckReplicaSets(unittest.TestCase):
         self, mock_get_cmd_result, mock_get_replica_set_name
     ):
         """Test rs.status() failure 1."""
-        mock_get_replica_set_name.return_value = False
+        mock_get_replica_set_name.return_value = "localhost", "27017", False
 
-        result = "CRITIAL: unable to determine the replica set name\n"
+        result = "CRITICAL: unable to determine the host, port and replica set name\n"
         with mock.patch("sys.stdout", new=StringIO()) as fake_std_out:
             try:
                 get_replica_status()
@@ -234,7 +287,7 @@ class TestCheckReplicaSets(unittest.TestCase):
         self, mock_get_cmd_result, mock_get_replica_set_name
     ):
         """Test rs.status() failure 2."""
-        mock_get_replica_set_name.return_value = "dont care"
+        mock_get_replica_set_name.return_value = "localhost", "27017", "dont care"
         mock_get_cmd_result.return_value = "{ this json is bogus }"
 
         result = "CRITICAL: failed to load json\n"