Merge ~sajoupa/charm-telegraf:sudoers-template into charm-telegraf:master

[Laurent Sesquès]

make sudoers files templates, taking into account install_method

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

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

[Laurent Sesquès (sajoupa)]

PAsses `make test`, with a small local fix to zaza for https://github.com/openstack-charmers/zaza/issues/436

[Barry Price (barryprice)]

LGTM, +1 here

review: Approve

[Haw Loeung (hloeung)]

review: Approve (+1)

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

Change successfully merged at revision cd6438f0126130f9dec34891c1a5e062ad5c69d4

diff --git a/src/files/sudoers/telegraf_ovs b/src/files/sudoers/telegraf_ovs
deleted file mode 100644
index 6be9f54..0000000
--- a/src/files/sudoers/telegraf_ovs
+++ /dev/null
@@ -1,5 +0,0 @@
-Defaults:telegraf !requiretty
-
-telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-ofctl
-telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-vsctl
-telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-appctl
diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
index 1fbe002..469b961 100644
--- a/src/reactive/telegraf.py
+++ b/src/reactive/telegraf.py
@@ -21,7 +21,6 @@ import ipaddress
 import json
 import os
 import re
-import shutil
 import socket
 import subprocess
 import sys
@@ -73,10 +72,12 @@ GRAFANA_DASHBOARD_NAME = "telegraf"
 SNAP_SERVICE = "snap.telegraf.telegraf"
 DEB_SERVICE = "telegraf"
 
-SNAP_USER = "snap_daemon"
+SNAP_OWNER = "snap_daemon"
 SNAP_GROUP = "snap_daemon"
-DEB_USER = "root"
+SNAP_USER = "snap_daemon"
+DEB_OWNER = "root"
 DEB_GROUP = "telegraf"
+DEB_USER = "telegraf"
 
 # Utilities #
 
@@ -93,7 +94,7 @@ def write_telegraf_file(path, content):
     return host.write_file(
         path,
         content.encode("UTF-8"),
-        owner=get_telegraf_user(),
+        owner=get_telegraf_owner(),
         group=get_telegraf_group(),
         perms=0o640,
     )
@@ -112,13 +113,18 @@ def get_install_method():
         raise InvalidInstallMethod()
 
 
-def install_sudoers_file(filename):
-    """Install a sudoers file from the charm directory to /etc/sudoers.d ."""
+def render_sudoers_file(filename):
+    """Generate and install a sudoers file from a template to /etc/sudoers.d ."""
+    template = "{}.tmpl".format(filename)
+    context = {"telegraf_user": get_telegraf_user()}
     hookenv.log("Installing sudoers file {}".format(filename), level="DEBUG")
-    src = os.path.join(get_files_dir(), "sudoers", filename)
-    dst = os.path.join(SUDOERS_DIR, filename)
-    shutil.copy(src, dst)
-    os.chmod(dst, 0o640)
+    render(
+        source=template,
+        templates_dir=os.path.join(get_templates_dir(), "sudoers"),
+        target=os.path.join(SUDOERS_DIR, filename),
+        context=context,
+        perms=0o640,
+    )
 
 
 def remove_sudoers_file(filename):
@@ -137,6 +143,15 @@ def get_telegraf_user():
     return DEB_USER
 
 
+def get_telegraf_owner():
+    install_method = get_install_method()
+
+    # get_install_method() already checked that we got a valid value
+    if install_method == "snap":
+        return SNAP_OWNER
+    return DEB_OWNER
+
+
 def get_telegraf_group():
     install_method = get_install_method()
 
@@ -597,7 +612,7 @@ def configure_telegraf():  # noqa: C901
     if host.service_running("openvswitch-switch"):
         # add sudoers file for telegraf if openvswitch is running
         sudoers_filename = "telegraf_ovs"
-        install_sudoers_file(sudoers_filename)
+        render_sudoers_file(sudoers_filename)
     else:
         # disable the OVS checks if the service is not curently running
         # no need to handle duplicates here, as those are handled during
@@ -622,7 +637,7 @@ def configure_telegraf():  # noqa: C901
     # handle the sudoers for iptables
     sudoers_filename = "telegraf_iptables"
     if config["collect_iptables_metrics"]:
-        install_sudoers_file(sudoers_filename)
+        render_sudoers_file(sudoers_filename)
     else:
         remove_sudoers_file(sudoers_filename)
 
diff --git a/src/files/sudoers/telegraf_iptables b/src/templates/sudoers/telegraf_iptables.tmpl
similarity index 66%
rename from src/files/sudoers/telegraf_iptables
rename to src/templates/sudoers/telegraf_iptables.tmpl
index a917a6f..49354db 100644
--- a/src/files/sudoers/telegraf_iptables
+++ b/src/templates/sudoers/telegraf_iptables.tmpl
@@ -1,3 +1,4 @@
 Cmnd_Alias IPTABLESSHOW = /sbin/iptables -w 5 -nvL *
-telegraf  ALL=(root) NOPASSWD: IPTABLESSHOW
+{{ telegraf_user }}  ALL=(root) NOPASSWD: IPTABLESSHOW
 Defaults!IPTABLESSHOW !logfile, !syslog, !pam_session
+
diff --git a/src/templates/sudoers/telegraf_ovs.tmpl b/src/templates/sudoers/telegraf_ovs.tmpl
new file mode 100644
index 0000000..b15f8ee
--- /dev/null
+++ b/src/templates/sudoers/telegraf_ovs.tmpl
@@ -0,0 +1,6 @@
+Defaults:{{ telegraf_user }} !requiretty
+
+{{ telegraf_user }} ALL = (root) NOPASSWD: /usr/bin/ovs-ofctl
+{{ telegraf_user }} ALL = (root) NOPASSWD: /usr/bin/ovs-vsctl
+{{ telegraf_user }} ALL = (root) NOPASSWD: /usr/bin/ovs-appctl
+
diff --git a/src/tests/lint/render_sudoers.py b/src/tests/lint/render_sudoers.py
new file mode 100644
index 0000000..4005ce1
--- /dev/null
+++ b/src/tests/lint/render_sudoers.py
@@ -0,0 +1,12 @@
+import os
+
+from jinja2 import Environment, FileSystemLoader
+
+template_dir = os.path.join("templates/sudoers")
+for template in os.listdir(template_dir):
+    environment = Environment(loader=FileSystemLoader(template_dir))
+    template = environment.get_template(template)
+    # A simple print(telegraf_user="test-user") would append a newline.
+    # We want to detect if the rendered template lacks a trailing
+    # newline, as it would break sudo when deployed. Hence end=''.
+    print(template.render(telegraf_user="test-user"), end="")
diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
index 537293d..d466780 100644
--- a/src/tests/unit/test_telegraf.py
+++ b/src/tests/unit/test_telegraf.py
@@ -201,10 +201,10 @@ def check_sysstat_config(original, expected):
 def test_write_telegraf_file(mocker, install_method):
 
     if install_method == "deb":
-        user = telegraf.DEB_USER
+        user = telegraf.DEB_OWNER
         group = telegraf.DEB_GROUP
     elif install_method == "snap":
-        user = telegraf.SNAP_USER
+        user = telegraf.SNAP_OWNER
         group = telegraf.SNAP_GROUP
     write_file = mocker.patch("charmhelpers.core.host.write_file")
     telegraf.write_telegraf_file("the-path", "the-content áéíóú")
@@ -1504,16 +1504,24 @@ class TestGrafanaDashboard:
         mock_clear_flag.assert_called_once_with("grafana.configured")
 
 
[e-mail]("shutil.copy")
[e-mail]("os.chmod")
-def test_install_sudoers_file(os_chmod_mock, shutil_copy_mock):
[e-mail]("install_method", ["deb", "snap"])
[e-mail]("reactive.telegraf.render")
+def test_render_sudoers_file(render_mock, install_method):
+    if install_method == "deb":
+        context = {"telegraf_user": "telegraf"}
+    elif install_method == "snap":
+        context = {"telegraf_user": "snap_daemon"}
     filename = "test"
+    template = "{}.tmpl".format(filename)
     target_mode = 0o640
-    src = os.path.join(telegraf.get_files_dir(), "sudoers", filename)
-    dst = os.path.join(telegraf.SUDOERS_DIR, filename)
-    telegraf.install_sudoers_file(filename)
-    shutil_copy_mock.assert_called_with(src, dst)
-    os_chmod_mock.assert_called_with(dst, target_mode)
+    telegraf.render_sudoers_file(filename)
+    render_mock.assert_called_with(
+        source=template,
+        templates_dir=os.path.join(telegraf.get_templates_dir(), "sudoers"),
+        target=os.path.join(telegraf.SUDOERS_DIR, filename),
+        context=context,
+        perms=target_mode,
+    )
 
 
 @mock.patch("os.unlink")
diff --git a/src/tox.ini b/src/tox.ini
index d9467a8..ec6b760 100644
--- a/src/tox.ini
+++ b/src/tox.ini
@@ -26,14 +26,15 @@ passenv =
 commands =
     flake8
     black --check --exclude "/(\.eggs|\.git|\.tox|\.venv|\.build|dist|charmhelpers|mod)/" .
-    bash -c "cat files/sudoers/* | visudo -csf -"
+    bash -c "python3 tests/lint/render_sudoers.py | visudo -csf -"
 deps =
     black
     flake8
+    flake8-colors
     flake8-docstrings
     flake8-import-order
+    jinja2
     pep8-naming
-    flake8-colors
 whitelist_externals = bash
 
 [flake8]