Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: node_exporter - Fix Systemd ProtectHome option in service unit #94

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

cudevmaxwell
Copy link
Contributor

Fixes an issue with the jinja2 snippet which is used to create the node_exporter Systemd unit. More details here: #13

Jinja2 namespaces are used to ensure the variable protect_home can be set in the parent scope of the for loop looking through the mounts.

@cudevmaxwell cudevmaxwell changed the title [bugfix] node_exporter: Fix Systemd ProtectHome option in service unit bugfix - node_exporter: Fix Systemd ProtectHome option in service unit May 3, 2023
@cudevmaxwell
Copy link
Contributor Author

Some testing at the REPL:

>>> import jinja2
>>> t = """
... {% set ns = namespace(protect_home = 'yes') %}
... {% for m in ansible_mounts if m.mount.startswith('/home') %}
... {%   set ns.protect_home = 'read-only' %}
... {% endfor %}
... ProtectHome={{ ns.protect_home }}
... NoNewPrivileges=yes
... """
>>> tmpl = jinja2.Template(t)
>>> tmpl.render(ansible_mounts=[{"mount":"/root"}, {"mount":"/"}])
'\n\n\nProtectHome=yes\nNoNewPrivileges=yes'
>>> tmpl.render(ansible_mounts=[{"mount":"/root"}, {"mount":"/"}, {"mount":"/home/here"}])
'\n\n\n\n\nProtectHome=read-only\nNoNewPrivileges=yes'
>>> tmpl.render(ansible_mounts=[{"mount":"/home"}, {"mount":"/root"}, {"mount":"/"}])
'\n\n\n\n\nProtectHome=read-only\nNoNewPrivileges=yes'

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it'd be good to add a test for the correct ProtectHome value in the molecule tests.

@SuperQ SuperQ changed the title bugfix - node_exporter: Fix Systemd ProtectHome option in service unit fix: node_exporter - Fix Systemd ProtectHome option in service unit May 5, 2023
@github-actions github-actions bot added the bugfix label May 5, 2023
@cudevmaxwell
Copy link
Contributor Author

I think I understand how to add a molecule test to ensure the service file exists or has the right contents (something like https://github.com/prometheus-community/ansible/blob/5a3968dd207a249b0841bf3e45000ac52f4ec991/roles/prometheus/molecule/alternative/tests/test_alternative.py#LL24C1-L35C21) but I'm not sure how to actually test this. Three machines would need to be spun up:

  • one with no special partitions, test that ProtectHome is set to 'yes'.
  • one with a partition mounted at /home, test that ProtectHome is set to 'read-only'.
  • one with a partition mounted at /home/some/other/path, test that ProtectHome is set to 'read-only'.

@SuperQ
Copy link
Contributor

SuperQ commented May 6, 2023

Yea, I was looking for some simple pytest options to assert "line in file", but I didn't find anything reasonable in the docs.

Maybe @gardar knows this stuff better.

@gardar
Copy link
Member

gardar commented May 9, 2023

Yea, I was looking for some simple pytest options to assert "line in file", but I didn't find anything reasonable in the docs.

Maybe @gardar knows this stuff better.

Something like file.contains ?

But perhaps systemd_properties would be even better.

@github-actions github-actions bot added bugfix and removed bugfix labels May 9, 2023
Fixes an issue with the jinja2 snippet which is used to create the node_exporter Systemd unit. More details here: #13

Jinja2 namespaces are used to ensure the variable `protect_home` can be set in the parent scope of the `for` loop looking through the mounts.

Signed-off-by: Kevin Bowrin <kevinbowrin@cunet.carleton.ca>
@github-actions github-actions bot added bugfix and removed bugfix labels May 9, 2023
@cudevmaxwell
Copy link
Contributor Author

I've added a simple test (thanks @gardar for pointing out systemd_properties), but it's only half-baked. I'm not sure where to hook into the molecule tests to have different filesystems mounted under /home or at /home.

@SuperQ SuperQ merged commit 51997ca into prometheus-community:main May 10, 2023
33 checks passed
@SuperQ
Copy link
Contributor

SuperQ commented May 10, 2023

Trying out some test changes here: #101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants