Merge ~mustafakemalgilor/charm-grafana:bug/1983814 into charm-grafana:master

[Mustafa Kemal Gilor]

dashboards_backup script compatibility update for Grafana version >=8:

The dashboards_backup script was using Slug API for retrieving the
dashboards, which is deprecated in Grafana version '5.4.0' in favor
of UID API, and removed in version '8.0.0'. Due to that, the backup
script was failing to generate backups in version 8.0.0 and above.

This fix introduces an API switch mechanism between Slug API and
UID API, which allows the script to behave as before for Grafana
(< 8.0.0) and use UID API for Grafana version (>= 8.0.0).

Fixes LP issue #1983814

dashboards_backup script compatibility update for Grafana version >=8:

The dashboards_backup script was using Slug API for retrieving the
dashboards, which is deprecated in Grafana version '5.4.0' in favor
of UID API, and removed in version '8.0.0'. Due to that, the backup
script was failing to generate backups in version 8.0.0 and above.

This fix introduces an API switch mechanism between Slug API and
UID API, which allows the script to behave as before for Grafana
(< 8.0.0) and use UID API for Grafana version (>= 8.0.0).

Fixes LP issue #1983814

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

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

[Robert Gildein (rgildein)]

Can we add at least some unit test for these changes.

review: Needs Fixing

[Mustafa Kemal Gilor (mustafakemalgilor)]

> Can we add at least some unit test for these changes.

Well, the original dashboards_backup code does not have any unit tests. I am implementing a bugfix, so in fact, I'll still not be writing any unit tests for this change if the code had them already. It is not my intention to implement unit tests from scratch. Don't get me wrong I *love* unit tests and am a strong believer in testing, but I don't necessarily believe I should take the burden in the scope of a simple bugfix... but anyways. I added unit tests for dashboards_backup.

[Robert Gildein (rgildein)]

Of course, if there are no tests and the change is negligible, it
is more acceptable that no tests are added. But in this case, a new
`get_backup_filename` function was added, so I don't see any reason
why unit tests should not be made for it. Also a change in the main
function was not negligible.

If every bugfix will add new functions without tests, then of course
we will never have any.

Thanks.

I tried to run the unit tests but they failed.
https://pastebin.ubuntu.com/p/Xgq98C88JP/

review: Needs Fixing

[Mustafa Kemal Gilor (mustafakemalgilor)]

> Of course, if there are no tests and the change is negligible, it
> is more acceptable that no tests are added. But in this case, a new
> `get_backup_filename` function was added, so I don't see any reason
> why unit tests should not be made for it. Also a change in the main
> function was not negligible.
>
> If every bugfix will add new functions without tests, then of course
> we will never have any.
>
> Thanks.
>
> I tried to run the unit tests but they failed.
> https://pastebin.ubuntu.com/p/Xgq98C88JP/

I've fixed the failing unit test, should be good to go. Also deployed & tested on queens/bionic with both snap & apt install methods; works as intended.

[Mustafa Kemal Gilor (mustafakemalgilor)]

Bump; still waiting for review & merge.

[Robert Gildein (rgildein)]

LGTM

review: Approve

[Mustafa Kemal Gilor (mustafakemalgilor)]

test results: https://pastebin.ubuntu.com/p/GswzFGhDZs/

[Giuseppe Petralia (peppepetra)]

LGTM, thanks!

review: Approve

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

Change successfully merged at revision 22ccaf7f9b76949809863ba4dd58d0188365410b

diff --git a/src/files/dashboards_backup b/src/files/dashboards_backup
deleted file mode 100755
index 88c6310..0000000
--- a/src/files/dashboards_backup
+++ /dev/null
@@ -1,46 +0,0 @@
-#!/usr/bin/python3
-
-import os
-import argparse
-import requests
-import string
-import datetime
-import json
-
-
-def get_backup_filename(directory, org_name, uri):
-    valid_chars = '-_(){}{}'.format(string.ascii_letters, string.digits)
-    dashboard = uri.split('/')[1]
-    filename = ''.join(c if c in valid_chars else '_' for c in dashboard)
-    filename += '.json.' + datetime.datetime.utcnow().strftime('%Y%m%d_%H%M%S')
-    org_name = ''.join(c if c in valid_chars else '_' for c in org_name)
-    return os.path.join(directory, org_name, filename)
-
-
-def main(args):
-    base_url = '{scheme}://localhost:{port}/api/'.format(scheme=args.scheme, port=args.port)
-    for key in args.api_keys:
-        headers = {'Authorization': 'Bearer {}'.format(key)}
-        org_name = requests.get(base_url+'org', headers=headers).json()['name']
-        r = requests.get(base_url+'search', headers=headers).json()
-        uris = [i['uri'] for i in r]
-        for d in uris:
-            backup_file = get_backup_filename(args.directory, org_name, d)
-            if not os.path.isdir(os.path.dirname(backup_file)):
-                os.mkdir(os.path.dirname(backup_file))
-            dashboard = requests.get(base_url+'dashboards/'+d, headers=headers).json()
-            with open(backup_file, 'w') as f:
-                f.write(json.dumps(dashboard))
-
-
-if __name__ == '__main__':
-    parser = argparse.ArgumentParser(description='Back up grafana dashboards to disk')
-    parser.add_argument('-d', '--directory', help='Directory where to store backups',
-                        default='/srv/backups')
-    parser.add_argument('-p', '--port', help='Port to access grafana API', default='3000')
-    parser.add_argument('-s', '--scheme',
-                        help='Scheme to use to access grafana API e.g. http or https',
-                        default='http')
-    parser.add_argument('api_keys', help='List of API keys to use for backups', nargs='+')
-    args = parser.parse_args()
-    main(args)
diff --git a/src/files/dashboards_backup.py b/src/files/dashboards_backup.py
new file mode 100755
index 0000000..a1abe64
--- /dev/null
+++ b/src/files/dashboards_backup.py
@@ -0,0 +1,86 @@
+#!/usr/bin/python3
+"""Grafana dashboards backup script."""
+
+import argparse
+import datetime
+import json
+import os
+import string
+
+import requests
+
+
+def get_backup_filename(directory, org_name, uri):
+    """Make a backup file name for dashboard."""
+    valid_chars = "-_(){}{}".format(string.ascii_letters, string.digits)
+    dashboard = uri.split("/")[1]
+    filename = "".join(c if c in valid_chars else "_" for c in dashboard)
+    filename += ".json." + datetime.datetime.utcnow().strftime("%Y%m%d_%H%M%S")
+    org_name = "".join(c if c in valid_chars else "_" for c in org_name)
+    return os.path.join(directory, org_name, filename)
+
+
+def should_use_uid_api(base_url, session):
+    """Determine the API to be used to retrieve the dashboard data."""
+    # Slug API is deprecated in 5.4.0 and removed in 8.0.0
+    # Switch to UID API if we're running in 8.0.0 and above
+    try:
+        fe_settings = session.get(base_url + "frontend/settings").json()
+        grafana_version_str = fe_settings["buildInfo"]["version"]
+        grafana_major_version = int(grafana_version_str.split(".")[0])
+        return grafana_major_version >= 8
+    except Exception as error:
+        print("Failed retrieving/parsing Grafana version string: " + str(error))
+        return False
+
+
+def main(args):
+    """Backup entry point."""
+    base_url = "{scheme}://localhost:{port}/api/".format(
+        scheme=args.scheme, port=args.port
+    )
+
+    for key in args.api_keys:
+        sess = requests.Session()
+        sess.headers.update({"Authorization": "Bearer {}".format(key)})
+
+        org_name = sess.get(base_url + "org").json()["name"]
+        search_result = sess.get(base_url + "search").json()
+
+        for elem in search_result:
+            dashboard_url = base_url + "dashboards/" + elem["uri"]
+
+            if should_use_uid_api(base_url, sess):
+                dashboard_url = base_url + "dashboards/uid/" + elem["uid"]
+                pass
+
+            backup_file = get_backup_filename(args.directory, org_name, elem["uri"])
+            if not os.path.isdir(os.path.dirname(backup_file)):
+                os.mkdir(os.path.dirname(backup_file))
+            dashboard = sess.get(dashboard_url).json()
+            with open(backup_file, "w") as f:
+                f.write(json.dumps(dashboard))
+
+
+if __name__ == "__main__":
+    parser = argparse.ArgumentParser(description="Back up grafana dashboards to disk")
+    parser.add_argument(
+        "-d",
+        "--directory",
+        help="Directory where to store backups",
+        default="/srv/backups",
+    )
+    parser.add_argument(
+        "-p", "--port", help="Port to access grafana API", default="3000"
+    )
+    parser.add_argument(
+        "-s",
+        "--scheme",
+        help="Scheme to use to access grafana API e.g. http or https",
+        default="http",
+    )
+    parser.add_argument(
+        "api_keys", help="List of API keys to use for backups", nargs="+"
+    )
+    args = parser.parse_args()
+    main(args)
diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
index dd70008..c9e6f28 100644
--- a/src/reactive/grafana.py
+++ b/src/reactive/grafana.py
@@ -600,8 +600,8 @@ def setup_backup_shedule():
         hookenv.status_set("maintenance", "Configuring grafana dashboard backup")
         hookenv.log("Setting up dashboards backup job...")
         host.rsync(
-            "files/dashboards_backup",
-            "/usr/local/bin/dashboards_backup",
+            "files/dashboards_backup.py",
+            "/usr/local/bin/dashboards_backup.py",
         )
         host.mkdir(config.get("dashboards_backup_dir"))
         settings = {
diff --git a/src/templates/juju-dashboards-backup.j2 b/src/templates/juju-dashboards-backup.j2
index 993c48a..86fc672 100644
--- a/src/templates/juju-dashboards-backup.j2
+++ b/src/templates/juju-dashboards-backup.j2
@@ -1,5 +1,5 @@
 #
 # Please do not edit. Juju will overwrite it.
 #
-{{ schedule }} root /usr/local/bin/dashboards_backup -d {{ directory }} -p {{ port }} -s {{ scheme }} {{ backup_keys }}
+{{ schedule }} root /usr/local/bin/dashboards_backup.py -d {{ directory }} -p {{ port }} -s {{ scheme }} {{ backup_keys }}
 
diff --git a/src/tests/unit/test_dashboards_backup.py b/src/tests/unit/test_dashboards_backup.py
new file mode 100644
index 0000000..3186d82
--- /dev/null
+++ b/src/tests/unit/test_dashboards_backup.py
@@ -0,0 +1,169 @@
+"""Unit test for dashboard_backup script."""
+
+import random
+import string
+import unittest
+from unittest import mock
+
+from files.dashboards_backup import get_backup_filename
+from files.dashboards_backup import main as invoke_backup
+from files.dashboards_backup import should_use_uid_api
+
+
+class MockResponse:
+    """Mock requests response data."""
+
+    def __init__(self, json_data, status_code):
+        """init."""
+        self.json_data = json_data
+        self.status_code = status_code
+
+    def json(self):
+        """json."""
+        return self.json_data
+
+
+# This method will be used by the mock to replace requests.get
+
+
+def mock_requests_get_pre_v8(*args, **kwargs):
+    """Mock API responses for pre-v8 Grafana."""
+    print("pre:" + args[0])
+    if "org" in args[0]:
+        return MockResponse({"name": "MainOrg"}, 200)
+    elif "search" in args[0]:
+        return MockResponse([{"uri": "dashboards/testdash", "uid": "testuid"}], 200)
+    elif "frontend/settings" in args[0]:
+        return MockResponse({"buildInfo": {"version": "7.9.9"}}, 200)
+    elif "dashboards/" in args[0]:
+        return MockResponse({"dashboard": "info"}, 200)
+    return MockResponse(None, 404)
+
+
+def mock_requests_get_post_v8(*args, **kwargs):
+    """Mock API responses for post-v8 Grafana."""
+    print("post:" + args[0])
+    if "frontend/settings" in args[0]:
+        return MockResponse({"buildInfo": {"version": "8.0.0"}}, 200)
+    elif "dashboards/uid/" in args[0]:
+        return MockResponse({"dashboard": "info-uid"}, 200)
+
+    return mock_requests_get_pre_v8(*args, **kwargs)
+
+
+def mock_requests_get_invalid_version(*args, **kwargs):
+    """Mock for returning an invalid version value."""
+    if "frontend/settings" in args[0]:
+        return MockResponse({"buildInfo": {"version": "invalid"}}, 200)
+    return mock_requests_get_pre_v8(*args, **kwargs)
+
+
+class GrafanaDashboardBackupTestCase(unittest.TestCase):
+    """Unit tests class for Dashboard Backup."""
+
+    def test_verify_backup_filename_sanity(self):
+        """Verify backup filename is sanitized properly."""
+        # The backup filename should be in form of
+        # directory/org_name/filename where all non-numeric,
+        # non-ascii characters replaced with an underscore.,
+        exceptions = ["_", "-", "(", ")"]
+        non_allowed_chars = "".join(
+            filter(
+                lambda item: item not in exceptions,
+                (string.punctuation + string.whitespace),
+            )
+        )
+        print(non_allowed_chars)
+        dir = "rootdir"
+        org = "default{}rg".format(random.choice(non_allowed_chars))
+        uri = "dashboards/da{}hboardslug".format(random.choice(non_allowed_chars))
+        result = get_backup_filename(dir, org, uri)
+        # Ensure all non-allowed chars are replaced
+        self.assertFalse(all(c in non_allowed_chars for c in result))
+        # Verify the count of replaced characters. 2(+1), +1 is for date suffix
+        self.assertEqual(result.count("_"), 3)
+
+    @mock.patch("files.dashboards_backup.requests.Session")
+    def test_should_use_uid_api_false(self, mock_session):
+        """Test should_use_uid_api with (<8.0.0) version number."""
+        mock_session.get.return_value = MockResponse(
+            {"buildInfo": {"version": "7.9.9"}}, 200
+        )
+        self.assertFalse(should_use_uid_api("http://test/a", mock_session))
+
+    @mock.patch("files.dashboards_backup.requests.Session")
+    def test_should_use_uid_api_true(self, mock_session):
+        """Test should_use_uid_api with (>=8.0.0) version number."""
+        mock_session.get.return_value = MockResponse(
+            {"buildInfo": {"version": "8.0.0"}}, 200
+        )
+        self.assertTrue(should_use_uid_api("http://test/a", mock_session))
+
+    @mock.patch(
+        "files.dashboards_backup.requests.Session.get",
+        side_effect=mock_requests_get_pre_v8,
+    )
+    @mock.patch("__main__.__builtins__.open", new_callable=mock.mock_open)
+    @mock.patch("files.dashboards_backup.os")
+    def test_verify_backup_slug_api(self, mock_os, mock_open, mock_get):
+        """Test main() against mock pre-v8 Grafana API."""
+
+        class DummyArgs(object):
+            pass
+
+        handle = mock_open()
+        args = DummyArgs()
+        setattr(args, "scheme", "http")
+        setattr(args, "port", "3000")
+        setattr(args, "api_keys", ["dummy-key-1"])
+        setattr(args, "directory", "backupdir")
+        mock_os.path.isdir.return_value = False
+        invoke_backup(args)
+        mock_os.mkdir.assert_called_once()
+        handle.write.assert_called_once_with(str('{"dashboard": "info"}'))
+
+    @mock.patch(
+        "files.dashboards_backup.requests.Session.get",
+        side_effect=mock_requests_get_post_v8,
+    )
+    @mock.patch("__main__.__builtins__.open", new_callable=mock.mock_open)
+    @mock.patch("files.dashboards_backup.os")
+    def test_verify_backup_uid_api(self, mock_os, mock_open, mock_get):
+        """Test main() against mock post-v8 Grafana API."""
+
+        class DummyArgs(object):
+            pass
+
+        handle = mock_open()
+        args = DummyArgs()
+        setattr(args, "scheme", "http")
+        setattr(args, "port", "3000")
+        setattr(args, "api_keys", ["dummy-key-1"])
+        setattr(args, "directory", "backupdir")
+        mock_os.path.isdir.return_value = False
+        invoke_backup(args)
+        mock_os.mkdir.assert_called_once()
+        handle.write.assert_called_once_with(str('{"dashboard": "info-uid"}'))
+
+    @mock.patch(
+        "files.dashboards_backup.requests.Session.get",
+        side_effect=mock_requests_get_invalid_version,
+    )
+    @mock.patch("__main__.__builtins__.open", new_callable=mock.mock_open)
+    @mock.patch("files.dashboards_backup.os")
+    def test_verify_backup_invalid_version(self, mock_os, mock_open, mock_get):
+        """Test main() against invalid Grafana version."""
+
+        class DummyArgs(object):
+            pass
+
+        handle = mock_open()
+        args = DummyArgs()
+        setattr(args, "scheme", "http")
+        setattr(args, "port", "3000")
+        setattr(args, "api_keys", ["dummy-key-1"])
+        setattr(args, "directory", "backupdir")
+        mock_os.path.isdir.return_value = False
+        invoke_backup(args)
+        mock_os.mkdir.assert_called_once()
+        handle.write.assert_called_once_with(str('{"dashboard": "info"}'))
diff --git a/src/tox.ini b/src/tox.ini
index 8bdf81e..2ef2d7f 100644
--- a/src/tox.ini
+++ b/src/tox.ini
@@ -70,6 +70,7 @@ commands =
            --cov=actions \
            --cov=hooks \
            --cov=src \
+           --cov=files \
            --cov-report=term \
            --cov-report=annotate:report/annotated \
            --cov-report=html:report/html