Merge charm-telegraf:bug/1896467 into charm-telegraf:master

[Facundo Ciccioli]

Write config files containing credentials as telegraf-only readable

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

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

[Facundo Ciccioli (fandanbango)]

Unit tests: https://paste.ubuntu.com/p/mBVpZkgVfR/
Functional tests: https://paste.ubuntu.com/p/J6hRwYkqrF/

[Benjamin Allot (ballot)]

Usually, files in /etc are written with root as owner and possibly a group with group readable permission.

I think something like

host.write_file(
            config_path,
            f.getvalue().encode("UTF-8"),
            owner=root,
            group=TELEGRAF_USER,
            perms=0o640,
)

is more on par with what we expect in /etc.
(0o440 is a possibility if we really want to discourage people to edit the files)

However, I may miss something.
Not a hard block, just something I'm curious about.

[Facundo Ciccioli (fandanbango)]

As per zzehring verbal comment, I've factored out the writing of the sensitive files.

Changed also ownership to root and made group readable and root writable as per ballot's suggestion (I do agree with that that's the custom for /etc).

Unit tests: https://paste.ubuntu.com/p/zjQCY6XP3R/
Functional tests: https://paste.ubuntu.com/p/Hw25x5N2Sp/

[Paul Goins (vultaire)]

LGTM

review: Approve

[Adam Dyess (addyess)]

LGTM2

review: Approve

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

Change successfully merged at revision 42c814bf359e12064506ccde51088d8cdd68542d

diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
index 1b1208e..e24b7eb 100644
--- a/src/reactive/telegraf.py
+++ b/src/reactive/telegraf.py
@@ -79,6 +79,15 @@ class InvalidInstallMethod(Exception):
     pass
 
 
+def write_telegraf_file(path, content):
+    return host.write_file(
+        path,
+        content.encode("UTF-8"),
+        group="telegraf",
+        perms=0o640,
+    )
+
+
 def get_install_method():
     config = hookenv.config()
 
@@ -861,7 +870,7 @@ def render_mysql_tmpl(contexts):
         for ctx in contexts:
             f.write(render_template(template, ctx))
             f.write(render_extra_options("inputs", "mysql"))
-        host.write_file(config_path, f.getvalue().encode("UTF-8"))
+        write_telegraf_file(config_path, f.getvalue())
     elif os.path.exists(config_path):
         os.unlink(config_path)
 
@@ -876,7 +885,7 @@ def render_postgresql_tmpl(contexts):
         ).read()
         for ctx in contexts:
             f.write(render_template(template, ctx))
-        host.write_file(config_path, f.getvalue().encode("UTF-8"))
+        write_telegraf_file(config_path, f.getvalue())
     elif os.path.exists(config_path):
         os.unlink(config_path)
 
@@ -917,7 +926,7 @@ def haproxy_input(haproxy):
             template, {"servers": json.dumps(haproxy_addresses)}
         ) + render_extra_options("inputs", "haproxy")
         hookenv.log("Updating {} plugin config file".format("haproxy"))
-        host.write_file(config_path, input_config.encode("utf-8"))
+        write_telegraf_file(config_path, input_config)
         set_flag("plugins.haproxy.configured")
     elif os.path.exists(config_path):
         os.unlink(config_path)
@@ -1123,7 +1132,7 @@ def rabbitmq_input(rabbitmq):
     )
 
     hookenv.log("Updating {} plugin config file".format("rabbitmq"))
-    host.write_file(config_path, input_config.encode("utf-8"))
+    write_telegraf_file(config_path, input_config)
 
     set_flag("plugins.rabbitmq.configured")
     set_flag("telegraf.needs_reload")
@@ -1168,7 +1177,7 @@ def influxdb_api_output(influxdb):
             },
         )
         extra_opts = render_extra_options("outputs", "influxdb")
-        host.write_file(config_path, "\n".join([content, extra_opts]).encode("utf-8"))
+        write_telegraf_file(config_path, "\n".join([content, extra_opts]))
         set_flag("plugins.influxdb-api.configured")
     elif os.path.exists(config_path):
         os.unlink(config_path)
diff --git a/src/tests/unit/test_mysql.py b/src/tests/unit/test_mysql.py
index 9015600..3de025e 100644
--- a/src/tests/unit/test_mysql.py
+++ b/src/tests/unit/test_mysql.py
@@ -64,11 +64,11 @@ class TestMySQL(unittest.TestCase):
         toggle_flag.assert_called_once_with("plugins.mysql.configured", True)
 
     @patch("reactive.telegraf.render_extra_options")
-    @patch("charmhelpers.core.host.write_file")
+    @patch("reactive.telegraf.write_telegraf_file")
     @patch("reactive.telegraf.get_templates_dir")
     @patch("reactive.telegraf.get_base_dir")
     def test_render_mysql_tmpl(
-        self, get_base_dir, get_templates_dir, write_file, extra_options
+        self, get_base_dir, get_templates_dir, write_telegraf_file, extra_options
     ):
         get_base_dir.return_value = "/etc/telegraf"
         get_templates_dir.return_value = os.path.join(
@@ -89,7 +89,10 @@ class TestMySQL(unittest.TestCase):
             ]
         )
 
-        write_file.assert_called_once_with("/etc/telegraf/telegraf.d/mysql.conf", ANY)
+        write_telegraf_file.assert_called_once_with(
+            "/etc/telegraf/telegraf.d/mysql.conf",
+            ANY,
+        )
 
     @patch("os.unlink")
     @patch("os.path.exists")
diff --git a/src/tests/unit/test_postgresql.py b/src/tests/unit/test_postgresql.py
index c011b32..1eba510 100644
--- a/src/tests/unit/test_postgresql.py
+++ b/src/tests/unit/test_postgresql.py
@@ -113,10 +113,12 @@ class TestPostgreSQL(unittest.TestCase):
             [call("plugins.postgresql.configured"), call("telegraf.needs_reload")]
         )
 
-    @patch("charmhelpers.core.host.write_file")
+    @patch("reactive.telegraf.write_telegraf_file")
     @patch("reactive.telegraf.get_templates_dir")
     @patch("reactive.telegraf.get_base_dir")
-    def test_render_postgresql_tmpl(self, get_base_dir, get_templates_dir, write_file):
+    def test_render_postgresql_tmpl(
+        self, get_base_dir, get_templates_dir, write_telegraf_file
+    ):
         get_templates_dir.return_value = os.path.join(
             os.path.dirname(__file__), "../..", "templates"
         )
@@ -126,15 +128,16 @@ class TestPostgreSQL(unittest.TestCase):
             [{"replica": "master", "conn_str": "my_conn_str", "server": "my_server"}]
         )
 
-        write_file.assert_called_once_with(
-            "/etc/telegraf/telegraf.d/postgresql.conf", ANY
+        write_telegraf_file.assert_called_once_with(
+            "/etc/telegraf/telegraf.d/postgresql.conf",
+            ANY,
         )
 
-    @patch("charmhelpers.core.host.write_file")
+    @patch("reactive.telegraf.write_telegraf_file")
     @patch("reactive.telegraf.get_templates_dir")
     @patch("reactive.telegraf.get_base_dir")
     def test_render_postgresql_tmpl_extra_options(
-        self, get_base_dir, get_templates_dir, write_file
+        self, get_base_dir, get_templates_dir, write_telegraf_file
     ):
         get_templates_dir.return_value = os.path.join(
             os.path.dirname(__file__), "../..", "templates"
@@ -159,8 +162,8 @@ class TestPostgreSQL(unittest.TestCase):
         ) as fd:
             rendered = telegraf.render_template(fd.read(), context)
 
-        write_file.assert_called_once_with(
-            "/etc/telegraf/telegraf.d/postgresql.conf", rendered.encode("UTF-8")
+        write_telegraf_file.assert_called_once_with(
+            "/etc/telegraf/telegraf.d/postgresql.conf", rendered
         )
         # now check if what's there is the thing we actually expect
         expected = """[[inputs.postgresql_extensible]]
diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
index 8ef82ec..a15b5b6 100644
--- a/src/tests/unit/test_telegraf.py
+++ b/src/tests/unit/test_telegraf.py
@@ -190,6 +190,17 @@ def check_sysstat_config(original, expected):
     assert actual == expected
 
 
+def test_write_telegraf_file(mocker):
+    write_file = mocker.patch("charmhelpers.core.host.write_file")
+    telegraf.write_telegraf_file("the-path", "the-content áéíóú")
+    write_file.assert_called_once_with(
+        "the-path",
+        "the-content áéíóú".encode("utf-8"),
+        group="telegraf",
+        perms=0o640,
+    )
+
+
 def test_sadc_options_correct(monkeypatch):
     """If SADC_OPTIONS is already correct, should return None."""
     original = dedent(