Merge ~xavpaice/charm-grafana:lp1947669 into charm-grafana:master

[Xav Paice]

Update permissions on grafana.db on access

    If Juju accesses the sqlite db for Grafana prior to Grafana creating it,
    the file is created with root ownership and Grafana cannot write to the
    db. This change adds a db access function with chown.

    Fixes bug LP: #1947669

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:f051fd73411d5b77d30dde7bfe529fb8c9e54aa9
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/47/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/148/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/597/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/47//rebuild

review: Approve (continuous-integration)

[Alvaro Uria (aluria)]

Please find a command below about "chowning" data_dir with the "grafana" user and group. I don't recall if that would work on snap-deployed envs (the snap does not create such users, as the "apt" method does).

[Alvaro Uria (aluria)]

review: Needs Information

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:d2bfdec0c4180ab4703247437868e2a35d7dda4d
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/48/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/149/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/601/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/48//rebuild

review: Approve (continuous-integration)

[Xav Paice (xavpaice)]

Thanks Alvaro, indeed the snap runs grafana as the root user, and there's no grafana user. I've updated the change to reflect that.

[Andrea Ieri (aieri)]

lgtm

review: Approve

[Zachary Zehring (zzehring)]

Small comment inline.

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:203365ad7974052c4c3610850f9c0e4c844f8228
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/49/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/150/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/603/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/49//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:a70eca09fe4475d3124e13f24dceeb07caef7220
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/50/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/151/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/604/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/50//rebuild

review: Approve (continuous-integration)

[Xav Paice (xavpaice)]

Squashed commits, added unit test, fixed typo (not using 'path' when I'd just set it), made the chmod conditional on apt install only.

[Zachary Zehring (zzehring)]

Very small comment inline. Otherwise LGTM

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:a17ded6b7b11dd0d2873a5a59b8cd8d5dbf9ebd3
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/51/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/152/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/608/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/51//rebuild

review: Approve (continuous-integration)

[Zachary Zehring (zzehring)]

+1

review: Approve

[Andrea Ieri (aieri)]

two small comments inline

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:ad9efd5d18b3e808d0d752e0305a16e6459c85fb
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/52/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit-withcompiler/153/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/610/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-grafana-ci/52//rebuild

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

+1

review: Approve

[Alvaro Uria (aluria)]

review: Approve

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

Change successfully merged at revision 17248daa461de95560156c1fc210331c2a8c1d40

diff --git a/src/reactive/grafana.py b/src/reactive/grafana.py
index 0a96d4f..9672653 100644
--- a/src/reactive/grafana.py
+++ b/src/reactive/grafana.py
@@ -17,6 +17,8 @@ wipe_nrpe_checks (no nrpe-external-master.available)
 [Helper funcs]
     * data_path(): retrieves /var/lib/grafana or /var/snap/grafana depending on
     install_method
+    * db_connection(): returns a sqlite3 Connection object for the Grafana database,
+    setting permissions and owner if appropriate
     * check_ports(new_port): close-port (if new_port) and open-port
     * select_query(query, params=None): helper to return rows from a query
     * insert_query(query, params=None): helper to update the db
@@ -382,6 +384,21 @@ def check_ports(new_port):
         kv.set("grafana.port", new_port)
 
 
+def db_connection():
+    """Return a connection to the Grafana sqlite db."""
+    data_dir = data_path()
+    if not data_dir:
+        return None
+    path = "{}/grafana.db".format(data_dir)
+    conn = sqlite3.connect(path, timeout=30)
+    source = get_install_source()
+    if "root" in host.owner(path) and source == "apt":
+        # Grafana runs as 'grafana', but Juju has made the grafana.db file as root.
+        host.chownr(data_dir, "grafana", "grafana")
+        os.chmod(path, 0o640)
+    return conn
+
+
 def select_query(query, params=None):
     """Return rows for a query.
 
@@ -391,10 +408,9 @@ def select_query(query, params=None):
     :type params: list of strings
     :rtype: list of rows found
     """
-    data_dir = data_path()
-    if not data_dir:
+    conn = db_connection()
+    if not conn:
         return
-    conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
     cur = conn.cursor()
     if params:
         return cur.execute(query, params).fetchall()
@@ -411,10 +427,9 @@ def insert_query(query, params=None):
     :type params: list of strings
     :rtype: None
     """
-    data_dir = data_path()
-    if not data_dir:
+    conn = db_connection()
+    if not conn:
         return
-    conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
     cur = conn.cursor()
     if params:
         cur.execute(query, params)
@@ -704,7 +719,9 @@ def sources_gone(relation):
     hookenv.log(ds)
 
     if ds:
-        conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
+        conn = db_connection()
+        if not conn:
+            return
         cur = conn.cursor()
         cur.execute(
             "DELETE FROM DATA_SOURCE WHERE  type=? AND url=?",
@@ -769,10 +786,9 @@ def check_datasource(ds):
     #  'username': 'username,
     #  'password': 'password
     # }
-    data_dir = data_path()
-    if not data_dir:
+    conn = db_connection()
+    if not conn:
         return
-    conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
     cur = conn.cursor()
     query = cur.execute("SELECT id, type, name, url, is_default FROM DATA_SOURCE")
     rows = query.fetchall()
@@ -1223,10 +1239,9 @@ def check_adminuser():
         stmt += ", password=?, theme='light'"
         stmt += " WHERE id = ?"
 
-        data_dir = data_path()
-        if not data_dir:
+        conn = db_connection()
+        if not conn:
             return
-        conn = sqlite3.connect("{}/grafana.db".format(data_dir), timeout=30)
         cur = conn.cursor()
         # Check if we have more than one user in the db
         for x in range(0, 5):
diff --git a/src/tests/unit/test_grafana.py b/src/tests/unit/test_grafana.py
index 7f4a53b..f15c9e9 100644
--- a/src/tests/unit/test_grafana.py
+++ b/src/tests/unit/test_grafana.py
@@ -321,3 +321,53 @@ class GrafanaTestCase(unittest.TestCase):
                 "group": group,
             },
         )
+
+    @mock.patch("sqlite3.connect")
+    @mock.patch("os.chmod")
+    @mock.patch("charmhelpers.core.host.chownr")
+    @mock.patch.object(grafana_reactive, "data_path", auto_spec=True)
+    @mock.patch("charmhelpers.core.host.owner")
+    @mock.patch.object(grafana_reactive, "get_install_source", auto_spec=True)
+    def test_db_connection_snap(
+        self,
+        mock_install_source,
+        mock_owner,
+        mock_data_path,
+        mock_chownr,
+        mock_chmod,
+        mock_sqlite3,
+    ):
+        """Test that the db_connection leaves perms on grafana.db."""
+        mock_install_source.return_value = "snap"
+        mock_data_path.return_value = "/var/snap/grafana"
+        mock_owner.return_value = "root"
+        mock_sqlite3.return_value = True
+        connection = grafana_reactive.db_connection()
+        self.assertTrue(connection)
+        mock_chmod.assert_not_called()
+        mock_chownr.assert_not_called()
+
+    @mock.patch("sqlite3.connect")
+    @mock.patch("os.chmod")
+    @mock.patch("charmhelpers.core.host.chownr")
+    @mock.patch.object(grafana_reactive, "data_path", auto_spec=True)
+    @mock.patch("charmhelpers.core.host.owner")
+    @mock.patch.object(grafana_reactive, "get_install_source", auto_spec=True)
+    def test_db_connection_apt(
+        self,
+        mock_install_source,
+        mock_owner,
+        mock_data_path,
+        mock_chownr,
+        mock_chmod,
+        mock_sqlite3,
+    ):
+        """Test that the db_connection fixes perms on grafana.db."""
+        mock_install_source.return_value = "apt"
+        mock_data_path.return_value = "/var/apt/grafana"
+        mock_owner.return_value = "root"
+        mock_sqlite3.return_value = True
+        connection = grafana_reactive.db_connection()
+        self.assertTrue(connection)
+        mock_chmod.assert_called()
+        mock_chownr.assert_called()