Merge pull request #9 from NETWAYS/chore/add-tests

Add pylint and unittests

.github/dependabot.yml

@@ -0,0 +1,6 @@
+version: 2
+updates:
+- package-ecosystem: "github-actions"
+  directory: "/"
+  schedule:
+    interval: monthly

.github/workflows/test.yml

@@ -0,0 +1,24 @@
+name: CI
+
+on: [push, pull_request]
+
+jobs:
+  gitHubActionForPytest:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        python-version: [3.9]
+    name: GitHub Action
+    steps:
+    - name: Checkout
+      uses: actions/checkout@v3
+    - name: Install dependencies
+      run: |
+        python -m pip install -r requirements.txt
+        python -m pip install -r requirements-dev.txt
+    - name: Lint
+      run: |
+        make lint
+    - name: Unittest
+      run: |
+        make coverage

.gitignore

@@ -1 +1,5 @@
 *.pyc
+venv/
+.venv/
+__pycache__/
+.coverage

.pylintrc

@@ -0,0 +1,15 @@
+# pylint config
+# [FORMAT]
+# good-names=m,p,l,ok
+[MESSAGES CONTROL]
+disable=fixme,
+        line-too-long,
+        too-many-locals,
+        too-many-arguments,
+        too-many-statements,
+        too-many-branches,
+        bare-except,
+        missing-module-docstring,
+        missing-function-docstring,
+        missing-class-docstring,
+        consider-using-f-string

CONTRIBUTING.md

@@ -0,0 +1,25 @@
+# Contributing
+Before starting your work on this module, you should [fork the project] to your GitHub account. This allows you to
+freely experiment with your changes. When your changes are complete, submit a [pull request]. All pull requests will be
+reviewed and merged if they suit some general guidelines:
+
+* Changes are located in a topic branch
+* For new functionality, proper tests are written
+* Changes should not solve certain problems on special environments
+
+## Branches
+Choosing a proper name for a branch helps us identify its purpose and possibly find an associated bug or feature.
+Generally a branch name should include a topic such as `fix` or `feature` followed by a description and an issue number
+if applicable. Branches should have only changes relevant to a specific issue.
+
+```
+git checkout -b fix/service-template-typo-1234
+git checkout -b feature/config-handling-1235
+git checkout -b doc/fix-typo-1236
+```
+
+## Testing
+Python modules are unit tested with the Python Standard Library. When modifying existing modules or tasks, make sure all existing tests pass. If you add new functionality, make sure to write appropriate tests as well.
+
+[fork the project]: https://help.github.com/articles/fork-a-repo/
+[pull request]: https://help.github.com/articles/using-pull-requests/

Makefile

@@ -0,0 +1,9 @@
+.PHONY: lint test coverage
+
+lint:
+	python -m pylint check_vmware_nsxt.py
+test:
+	env TZ=UTC python -m unittest -v -b test_check_vmware_nsxt.py
+coverage:
+	env TZ=UTC python -m coverage run -m unittest test_check_vmware_nsxt.py
+	env TZ=UTC python -m coverage report -m --include check_vmware_nsxt.py

check_vmware_nsxt.py

@@ -89,8 +89,6 @@ class Client:
     API_PREFIX = '/api/v1/'
 
     def __init__(self, api, username, password, logger=None, verify=True, max_age=5):
-        # TODO: parse and validate url?
-
         self.api = api
         self.username = username
         self.password = password
@@ -115,17 +113,18 @@ def request(self, url, method='GET'):
         self.logger.debug("starting API %s request from: %s", method, url)
 
         try:
-            response = requests.request(method, request_url, auth=HTTPBasicAuth(self.username, self.password), verify=self.verify)
+            response = requests.request(method, request_url, auth=HTTPBasicAuth(self.username, self.password), verify=self.verify, timeout=10)
         except requests.exceptions.RequestException as req_exc:
-            raise CriticalException(req_exc)
+            raise CriticalException(req_exc) # pylint: disable=raise-missing-from
 
         if response.status_code != 200:
+            # TODO What about 300 Redirects?
             raise CriticalException('Request to %s was not successful: %s' % (request_url, response.status_code))
 
         try:
             return response.json()
-        except Exception as e:
-            raise CriticalException('Could not decode API JSON: ' + str(e))
+        except Exception as json_exc:
+            raise CriticalException('Could not decode API JSON: ' + str(json_exc)) # pylint: disable=raise-missing-from
 
     def get_cluster_status(self):
         """
@@ -261,8 +260,8 @@ def build_output(self):
         self.summary.append("%d alarms" % count)
         self.perfdata.append("alarms=%d;;;0" % count)
 
-        for state in states:
-            self.summary.append("%d %s" % (states[state], state.lower()))
+        for state, value in states.items():
+            self.summary.append("%d %s" % (value, state.lower()))
 
     def build_status(self):
         states = []
@@ -320,8 +319,8 @@ def build_output(self):
             # Maybe we need count at some point...
             # self.perfdata.append("%s_count=%d;;;0;%d" % (label, usage['current_usage_count'], usage['max_supported_count']))
 
-        for state in states:
-            self.summary.append("%d %s" % (states[state], state.lower()))
+        for state, value in states.items():
+            self.summary.append("%d %s" % (value, state.lower()))
 
         if len(states) == 0:
             self.summary.append("no usages")
@@ -385,64 +384,58 @@ def worst_state(*states):
     return overall
 
 
-def parse_args():
-    args = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawTextHelpFormatter)
+def commandline(args):
+    parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawTextHelpFormatter)
 
-    args.add_argument('--api', '-A', required=True,
+    parser.add_argument('--api', '-A', required=True,
         help='VMware NSX-T URL without any sub-path (e.g. https://vmware-nsx.local)')
-
-    args.add_argument('--username', '-u', help='Username for Basic Auth', required=True)
-    args.add_argument('--password', '-p', help='Password for Basic Auth', required=True)
-
-    args.add_argument('--mode', '-m', help='Check mode', required=True)
-
-    args.add_argument('--max-age', '-M', help='Max age in minutes for capacity usage updates. Defaults to 5', default=5, required=False)
-
-    args.add_argument('--version', '-V', help='Print version', action='store_true')
-
-    args.add_argument('--insecure', help='Do not verify TLS certificate. Be careful with this option, please', action='store_true', required=False)
-
-    return args.parse_args()
-
-
-def main():
+    parser.add_argument('--username', '-u',
+                        help='Username for Basic Auth', required=True)
+    parser.add_argument('--password', '-p',
+                        help='Password for Basic Auth', required=True)
+    parser.add_argument('--mode', '-m', choices=['cluster-status', 'alarms', 'capacity-usage'],
+                        help='Check mode to exectue', required=True)
+    parser.add_argument('--max-age', '-M', type=int,
+                        help='Max age in minutes for capacity usage updates. Defaults to 5', default=5, required=False)
+    parser.add_argument('--insecure',
+                        help='Do not verify TLS certificate. Be careful with this option, please', action='store_true', required=False)
+    parser.add_argument('--version', '-V',
+                        help='Print version', action='store_true')
+
+    return parser.parse_args(args)
+
+
+def main(args):
     fix_tls_cert_store()
 
-    args = parse_args()
     if args.insecure:
         urllib3.disable_warnings()
 
     if args.version:
         print("check_vmware_nsxt version %s" % VERSION)
-        return 0
+        return 3
 
-    client = Client(args.api, args.username, args.password, verify=(not args.insecure), max_age=int(args.max_age))
+    client = Client(args.api, args.username, args.password, verify=(not args.insecure), max_age=args.max_age)
 
     if args.mode == 'cluster-status':
         return client.get_cluster_status().print_and_return()
-    elif args.mode == 'alarms':
+    if args.mode == 'alarms':
         return client.get_alarms().print_and_return()
-    elif args.mode == 'capacity-usage':
+    if args.mode == 'capacity-usage':
         return client.get_capacity_usage().print_and_return()
 
     print("[UNKNOWN] unknown mode %s" % args.mode)
     return UNKNOWN
 
 
-if __package__ == '__main__' or __package__ is None:
+if __package__ == '__main__' or __package__ is None: # pragma: no cover
     try:
-        sys.exit(main())
-    except CriticalException as e:
-        print("[CRITICAL] " + str(e))
+        ARGS = commandline(sys.argv[1:])
+        sys.exit(main(ARGS))
+    except CriticalException as main_exc:
+        print("[CRITICAL] " + str(main_exc))
         sys.exit(CRITICAL)
-    except Exception:
+    except Exception: # pylint: disable=broad-except
         exception = sys.exc_info()
         print("[UNKNOWN] Unexpected Python error: %s %s" % (exception[0], exception[1]))
-
-        try:
-            import traceback
-            traceback.print_tb(exception[2])
-        except:
-            pass
-
         sys.exit(UNKNOWN)

requirements-dev.txt

@@ -0,0 +1,2 @@
+pylint==2.15.0
+coverage==7.0.5

test_check_vmware_nsxt.py

@@ -0,0 +1,148 @@
+#!/usr/bin/env python3
+
+import unittest
+import unittest.mock as mock
+import sys
+
+import os
+import datetime
+import json
+
+sys.path.append('..')
+
+from check_vmware_nsxt import commandline
+from check_vmware_nsxt import worst_state
+from check_vmware_nsxt import time_iso
+from check_vmware_nsxt import build_datetime
+from check_vmware_nsxt import Client
+from check_vmware_nsxt import CriticalException
+
+os.environ["TZ"] = "UTC"
+
+class CLITesting(unittest.TestCase):
+
+    def test_commandline(self):
+        actual = commandline(['-A', 'api', '-u', 'user', '-p', 'password', '-m', 'alarms'])
+        self.assertEqual(actual.username, 'user')
+        self.assertEqual(actual.api, 'api')
+        self.assertEqual(actual.password, 'password')
+        self.assertEqual(actual.mode, 'alarms')
+        self.assertFalse(actual.insecure)
+        self.assertEqual(actual.max_age, 5)
+
+class UtilTesting(unittest.TestCase):
+
+    def test_worst_state(self):
+
+        actual = worst_state()
+        expected = 3
+        self.assertEqual(actual, expected)
+
+        actual = worst_state(0,1,2)
+        expected = 2
+        self.assertEqual(actual, expected)
+
+        actual = worst_state(1,2,3,4)
+        expected = 3
+        self.assertEqual(actual, expected)
+
+        actual = worst_state(0,0,0,0)
+        expected = 0
+        self.assertEqual(actual, expected)
+
+    def test_build_datetime(self):
+
+        actual = build_datetime(1683988760)
+        expected = datetime.datetime(1970, 1, 20, 11, 46, 28, 760000)
+        self.assertEqual(actual, expected)
+
+    def test_time_iso(self):
+
+        actual = build_datetime(1683988760)
+        expected = datetime.datetime(1970, 1, 20, 11, 46, 28, 760000)
+        self.assertEqual(actual, expected)
+
+class ClientTesting(unittest.TestCase):
+
+    @mock.patch('requests.request')
+    def test_cluster_status_404(self, mock_req):
+        m = mock.MagicMock()
+        m.status_code = 404
+        mock_req.return_value = m
+
+        c = Client('api', 'username', 'password', logger=None, verify=True, max_age=5)
+
+        with self.assertRaises(CriticalException) as context:
+            c.get_cluster_status().print_and_return()
+
+    @mock.patch('requests.request')
+    def test_cluster_status_no_json(self, mock_req):
+        m = mock.MagicMock()
+        m.status_code = 200
+        m.json.side_effect = Exception("no json")
+        mock_req.return_value = m
+
+        c = Client('api', 'username', 'password', logger=None, verify=True, max_age=5)
+
+        with self.assertRaises(CriticalException) as context:
+            c.get_cluster_status().print_and_return()
+
+    @mock.patch('builtins.print')
+    @mock.patch('requests.request')
+    def test_cluster_status_ok(self, mock_req, mock_print):
+
+        with open('testdata/fixtures/cluster-status.json') as f:
+            testdata = json.load(f)
+
+        m = mock.MagicMock()
+        m.status_code = 200
+        m.json.return_value = testdata
+        mock_req.return_value = m
+
+        c = Client('api', 'username', 'password', logger=None, verify=True, max_age=5)
+
+        actual = c.get_cluster_status().print_and_return()
+        expected = 0
+
+        self.assertEqual(actual, expected)
+        mock_print.assert_called_with("[OK] control_cluster_status=STABLE - mgmt_cluster_status=STABLE - control_cluster_status=STABLE - nodes_online=3\n\n[OK] DATASTORE: STABLE - 3 members\n[OK] CLUSTER_BOOT_MANAGER: STABLE - 3 members\n[OK] CONTROLLER: STABLE - 3 members\n[OK] MANAGER: STABLE - 3 members\n[OK] POLICY: STABLE - 3 members\n[OK] HTTPS: STABLE - 3 members\n[OK] ASYNC_REPLICATOR: STABLE - 3 members\n[OK] MONITORING: STABLE - 3 members\n[OK] IDPS_REPORTING: STABLE - 3 members\n[OK] CORFU_NONCONFIG: STABLE - 3 members\n| nodes_online=3;;;0")
+
+    @mock.patch('builtins.print')
+    @mock.patch('requests.request')
+    def test_alarms_ok(self, mock_req, mock_print):
+
+        with open('testdata/fixtures/alarms.json') as f:
+            testdata = json.load(f)
+
+        m = mock.MagicMock()
+        m.status_code = 200
+        m.json.return_value = testdata
+        mock_req.return_value = m
+
+        c = Client('api', 'username', 'password', logger=None, verify=True, max_age=5)
+
+        actual = c.get_alarms().print_and_return()
+        expected = 1
+
+        self.assertEqual(actual, expected)
+        mock_print.assert_called_with('[WARNING] 1 alarms - 1 medium\n\n[MEDIUM] (2021-04-26 15:25:18) (node1) Intelligence Health/Storage Latency High - Intelligence node storage latency is high.\n| alarms=1;;;0')
+
+    @mock.patch('builtins.print')
+    @mock.patch('requests.request')
+    def test_capacity_usage_ok(self, mock_req, mock_print):
+
+        with open('testdata/fixtures/capacity-usage.json') as f:
+            testdata = json.load(f)
+
+        m = mock.MagicMock()
+        m.status_code = 200
+        m.json.return_value = testdata
+        mock_req.return_value = m
+
+        c = Client('api', 'username', 'password', logger=None, verify=True, max_age=5)
+
+        actual = c.get_capacity_usage().print_and_return()
+        expected = 1
+
+        self.assertEqual(actual, expected)
+        mock_print.assert_called_with('[WARNING] 28 info - last update: 2021-04-30 09:17:40 - last update older than 5 minutes\n\n[OK] [INFO] System-wide NAT rules: 0 of 25000 (0%)\n[OK] [INFO] Network Introspection Rules: 1 of 10000 (0.01%)\n[OK] [INFO] System-wide Endpoint Protection Enabled Hosts: 0 of 256 (0%)\n[OK] [INFO] Hypervisor Hosts: 18 of 1024 (1.75%)\n[OK] [INFO] System-wide Firewall Rules: 81 of 100000 (0.08%)\n[OK] [INFO] System-wide DHCP Pools: 0 of 10000 (0%)\n[OK] [INFO] System-wide Edge Nodes: 10 of 320 (3.12%)\n[OK] [INFO] Active Directory Domains (Identity Firewall): 0 of 4 (0%)\n[OK] [INFO] vSphere Clusters Prepared for NSX: 4 of 128 (3.12%)\n[OK] [INFO] Prefix-lists: 20 of 500 (4%)\n[OK] [INFO] Logical Switches: 12 of 10000 (0.12%)\n[OK] [INFO] System-wide Logical Switch Ports: 145 of 25000 (0.58%)\n[OK] [INFO] Active Directory Groups (Identity Firewall): 0 of 100000 (0%)\n[OK] [INFO] Distributed Firewall Rules: 75 of 100000 (0.07%)\n[OK] [INFO] System-wide Endpoint Protection Enabled Virtual Machines: 0 of 7500 (0%)\n[OK] [INFO] Distributed Firewall Sections: 23 of 10000 (0.23%)\n[OK] [INFO] Groups Based on IP Sets: 37 of 10000 (0.37%)\n[OK] [INFO] Edge Clusters: 3 of 160 (1.87%)\n[OK] [INFO] Tier-1 Logical Routers with NAT Enabled: 0 of 4000 (0%)\n[OK] [INFO] System-wide Firewall Sections: 29 of 10000 (0.29%)\n[OK] [INFO] Network Introspection Sections: 1 of 500 (0.2%)\n[OK] [INFO] Groups: 74 of 20000 (0.37%)\n[OK] [INFO] Tier-1 Logical Routers: 4 of 4000 (0.1%)\n[OK] [INFO] IP Sets: 37 of 10000 (0.37%)\n[OK] [INFO] Network Introspection Service Chains: 0 of 24 (0%)\n[OK] [INFO] Network Introspection Service Paths: 0 of 4000 (0%)\n[OK] [INFO] Tier-0 Logical Routers: 2 of 160 (1.25%)\n[OK] [INFO] DHCP Server Instances: 0 of 10000 (0%)\n| number_of_nat_rules=0%;70;100;0;100 number_of_si_rules=0.01%;70;100;0;100 number_of_gi_protected_hosts=0%;70;100;0;100 number_of_prepared_hosts=1.75%;70;100;0;100 number_of_firewall_rules=0.08%;70;100;0;100 number_of_dhcp_ip_pools=0%;70;100;0;100 number_of_edge_nodes=3.12%;70;100;0;100 number_of_active_directory_domains=0%;70;100;0;100 number_of_vcenter_clusters=3.12%;70;100;0;100 number_of_prefix_list=4%;70;100;0;100 number_of_logical_switches=0.12%;70;100;0;100 number_of_logical_ports=0.58%;70;100;0;100 number_of_active_directory_groups=0%;70;100;0;100 number_of_dfw_rules=0.07%;70;100;0;100 number_of_gi_protected_vms=0%;70;100;0;100 number_of_dfw_sections=0.23%;70;100;0;100 number_of_groups_based_on_ip_sets=0.37%;70;100;0;100 number_of_edge_clusters=1.87%;70;100;0;100 number_of_tier1_with_nat_rule=0%;70;100;0;100 number_of_firewall_sections=0.29%;70;100;0;100 number_of_si_sections=0.2%;70;100;0;100 number_of_nsgroup=0.37%;70;100;0;100 number_of_tier1_routers=0.1%;70;100;0;100 number_of_ipsets=0.37%;70;100;0;100 number_of_si_service_chains=0%;70;100;0;100 number_of_si_service_paths=0%;70;100;0;100 number_of_tier0_routers=1.25%;70;100;0;100 number_of_dhcp_servers=0%;70;100;0;100')