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

Proposed by Facundo Ciccioli
Status: Merged
Approved by: Alvaro Uria
Approved revision: 9ccd1a821c2b08a24e79dddfd38be5d12a13b68b
Merged at revision: 42c814bf359e12064506ccde51088d8cdd68542d
Proposed branch: charm-telegraf:bug/1896467
Merge into: charm-telegraf:master
Diff against target: 168 lines (+42/-16)
4 files modified
src/reactive/telegraf.py (+14/-5)
src/tests/unit/test_mysql.py (+6/-3)
src/tests/unit/test_postgresql.py (+11/-8)
src/tests/unit/test_telegraf.py (+11/-0)
Reviewer Review Type Date Requested Status
Adam Dyess Approve
Paul Goins Approve
Benjamin Allot Pending
Review via email: mp+391124@code.launchpad.net

Commit message

Write config files containing credentials as telegraf-only readable

To post a comment you must log in.

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

Benjamin Allot (ballot) wrote :

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) wrote :

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) wrote :

LGTM

review: Approve
Adam Dyess (addyess) wrote :

LGTM2

review: Approve

Change successfully merged at revision 42c814bf359e12064506ccde51088d8cdd68542d

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
2index 1b1208e..e24b7eb 100644
3--- a/src/reactive/telegraf.py
4+++ b/src/reactive/telegraf.py
5@@ -79,6 +79,15 @@ class InvalidInstallMethod(Exception):
6 pass
7
8
9+def write_telegraf_file(path, content):
10+ return host.write_file(
11+ path,
12+ content.encode("UTF-8"),
13+ group="telegraf",
14+ perms=0o640,
15+ )
16+
17+
18 def get_install_method():
19 config = hookenv.config()
20
21@@ -861,7 +870,7 @@ def render_mysql_tmpl(contexts):
22 for ctx in contexts:
23 f.write(render_template(template, ctx))
24 f.write(render_extra_options("inputs", "mysql"))
25- host.write_file(config_path, f.getvalue().encode("UTF-8"))
26+ write_telegraf_file(config_path, f.getvalue())
27 elif os.path.exists(config_path):
28 os.unlink(config_path)
29
30@@ -876,7 +885,7 @@ def render_postgresql_tmpl(contexts):
31 ).read()
32 for ctx in contexts:
33 f.write(render_template(template, ctx))
34- host.write_file(config_path, f.getvalue().encode("UTF-8"))
35+ write_telegraf_file(config_path, f.getvalue())
36 elif os.path.exists(config_path):
37 os.unlink(config_path)
38
39@@ -917,7 +926,7 @@ def haproxy_input(haproxy):
40 template, {"servers": json.dumps(haproxy_addresses)}
41 ) + render_extra_options("inputs", "haproxy")
42 hookenv.log("Updating {} plugin config file".format("haproxy"))
43- host.write_file(config_path, input_config.encode("utf-8"))
44+ write_telegraf_file(config_path, input_config)
45 set_flag("plugins.haproxy.configured")
46 elif os.path.exists(config_path):
47 os.unlink(config_path)
48@@ -1123,7 +1132,7 @@ def rabbitmq_input(rabbitmq):
49 )
50
51 hookenv.log("Updating {} plugin config file".format("rabbitmq"))
52- host.write_file(config_path, input_config.encode("utf-8"))
53+ write_telegraf_file(config_path, input_config)
54
55 set_flag("plugins.rabbitmq.configured")
56 set_flag("telegraf.needs_reload")
57@@ -1168,7 +1177,7 @@ def influxdb_api_output(influxdb):
58 },
59 )
60 extra_opts = render_extra_options("outputs", "influxdb")
61- host.write_file(config_path, "\n".join([content, extra_opts]).encode("utf-8"))
62+ write_telegraf_file(config_path, "\n".join([content, extra_opts]))
63 set_flag("plugins.influxdb-api.configured")
64 elif os.path.exists(config_path):
65 os.unlink(config_path)
66diff --git a/src/tests/unit/test_mysql.py b/src/tests/unit/test_mysql.py
67index 9015600..3de025e 100644
68--- a/src/tests/unit/test_mysql.py
69+++ b/src/tests/unit/test_mysql.py
70@@ -64,11 +64,11 @@ class TestMySQL(unittest.TestCase):
71 toggle_flag.assert_called_once_with("plugins.mysql.configured", True)
72
73 @patch("reactive.telegraf.render_extra_options")
74- @patch("charmhelpers.core.host.write_file")
75+ @patch("reactive.telegraf.write_telegraf_file")
76 @patch("reactive.telegraf.get_templates_dir")
77 @patch("reactive.telegraf.get_base_dir")
78 def test_render_mysql_tmpl(
79- self, get_base_dir, get_templates_dir, write_file, extra_options
80+ self, get_base_dir, get_templates_dir, write_telegraf_file, extra_options
81 ):
82 get_base_dir.return_value = "/etc/telegraf"
83 get_templates_dir.return_value = os.path.join(
84@@ -89,7 +89,10 @@ class TestMySQL(unittest.TestCase):
85 ]
86 )
87
88- write_file.assert_called_once_with("/etc/telegraf/telegraf.d/mysql.conf", ANY)
89+ write_telegraf_file.assert_called_once_with(
90+ "/etc/telegraf/telegraf.d/mysql.conf",
91+ ANY,
92+ )
93
94 @patch("os.unlink")
95 @patch("os.path.exists")
96diff --git a/src/tests/unit/test_postgresql.py b/src/tests/unit/test_postgresql.py
97index c011b32..1eba510 100644
98--- a/src/tests/unit/test_postgresql.py
99+++ b/src/tests/unit/test_postgresql.py
100@@ -113,10 +113,12 @@ class TestPostgreSQL(unittest.TestCase):
101 [call("plugins.postgresql.configured"), call("telegraf.needs_reload")]
102 )
103
104- @patch("charmhelpers.core.host.write_file")
105+ @patch("reactive.telegraf.write_telegraf_file")
106 @patch("reactive.telegraf.get_templates_dir")
107 @patch("reactive.telegraf.get_base_dir")
108- def test_render_postgresql_tmpl(self, get_base_dir, get_templates_dir, write_file):
109+ def test_render_postgresql_tmpl(
110+ self, get_base_dir, get_templates_dir, write_telegraf_file
111+ ):
112 get_templates_dir.return_value = os.path.join(
113 os.path.dirname(__file__), "../..", "templates"
114 )
115@@ -126,15 +128,16 @@ class TestPostgreSQL(unittest.TestCase):
116 [{"replica": "master", "conn_str": "my_conn_str", "server": "my_server"}]
117 )
118
119- write_file.assert_called_once_with(
120- "/etc/telegraf/telegraf.d/postgresql.conf", ANY
121+ write_telegraf_file.assert_called_once_with(
122+ "/etc/telegraf/telegraf.d/postgresql.conf",
123+ ANY,
124 )
125
126- @patch("charmhelpers.core.host.write_file")
127+ @patch("reactive.telegraf.write_telegraf_file")
128 @patch("reactive.telegraf.get_templates_dir")
129 @patch("reactive.telegraf.get_base_dir")
130 def test_render_postgresql_tmpl_extra_options(
131- self, get_base_dir, get_templates_dir, write_file
132+ self, get_base_dir, get_templates_dir, write_telegraf_file
133 ):
134 get_templates_dir.return_value = os.path.join(
135 os.path.dirname(__file__), "../..", "templates"
136@@ -159,8 +162,8 @@ class TestPostgreSQL(unittest.TestCase):
137 ) as fd:
138 rendered = telegraf.render_template(fd.read(), context)
139
140- write_file.assert_called_once_with(
141- "/etc/telegraf/telegraf.d/postgresql.conf", rendered.encode("UTF-8")
142+ write_telegraf_file.assert_called_once_with(
143+ "/etc/telegraf/telegraf.d/postgresql.conf", rendered
144 )
145 # now check if what's there is the thing we actually expect
146 expected = """[[inputs.postgresql_extensible]]
147diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
148index 8ef82ec..a15b5b6 100644
149--- a/src/tests/unit/test_telegraf.py
150+++ b/src/tests/unit/test_telegraf.py
151@@ -190,6 +190,17 @@ def check_sysstat_config(original, expected):
152 assert actual == expected
153
154
155+def test_write_telegraf_file(mocker):
156+ write_file = mocker.patch("charmhelpers.core.host.write_file")
157+ telegraf.write_telegraf_file("the-path", "the-content áéíóú")
158+ write_file.assert_called_once_with(
159+ "the-path",
160+ "the-content áéíóú".encode("utf-8"),
161+ group="telegraf",
162+ perms=0o640,
163+ )
164+
165+
166 def test_sadc_options_correct(monkeypatch):
167 """If SADC_OPTIONS is already correct, should return None."""
168 original = dedent(

Subscribers

People subscribed via source and target branches

to all changes: