Add node-exporter #13

akshaymankar · 2020-08-28T12:07:17Z

Fixes https://github.com/zinfra/backend-issues/issues/1556

roles/sft-server/meta/main.yml

* Add role to create CA and client certificates This role will likely only be used only for boostrapping an environment. It doesn't take care of certificate rotation yet, but it can be made to do so as and when required.

arianvp

Looks fine.

Would be nicer if we had some more central internal CA; and that we dont store it in S3; but babysteps. We have 3150 days to fix it :P

lucendio

I made some comments and have some questions:

Since you included everything related to metrics into this role, instead of putting it in a separate one, would it make sense to make it optional?

Can you elaborate on why the monitoring-certs role came into existence?

roles/sft-server/templates/sftd.vhost.conf.j2

roles/sft-server/tasks/traffic.yml

roles/sft-server/tasks/assert.yml

roles/sft-server/molecule/default/verify.yml

roles/sft-server/meta/main.yml

roles/monitoring-certs/tasks/main.yml

roles/sft-server/tasks/configure.yml

Remove documentation for obsolete 'metrics_fqdn'

Also: * comment out usage of use_common_name_for_san as it is not available in ansible-2.7

In ansible 2.7, when openssl_csr sees and empty file, it panics.

They don't work so nicely with ansible 2.7, they need exact timestamp and calculating that is not fun. Fortunately they default to 10 years anyways.

lucendio

Since you included everything related to metrics into this role, instead of putting it in a separate one, would it make sense to make it optional?

Can you elaborate on why the sft-monitoring-certs role came into existence?

Minor comments. Also, the two questions above remain.

roles/sft-monitoring-certs/tasks/main.yml

roles/sft-monitoring-certs/tasks/ca-cert.yml

arianvp · 2020-09-08T09:06:44Z

roles/sft-server/templates/nginx.service.override.j2

+
+# NOTE: defaults to 'inherit' which leads to error messages not
+#       appearing if nginx.conf is set to 'error_log: stderr'
+StandardError=journal


Why are we sleeping? Is it to work around the bug that is linked? (I don't see why) or is it causing the bug that is linked?

I'm surprised inherit doesn't work StandardOutput= defaults to journal and StandardError=inherit just means "Connect to the same fd as StandardOutput" Honestly sounds like a systemd bug to me. @flokli thoughts?

This wasn't added in this PR, but I fixed it anyways :)

@akshaymankar can you elaborate on why you removed this? The issue that you and I linked closes with Confirmed → Won't Fix. I experienced the issue just last week, which was when I introduced the suggested workaround.

@arianvp the line you commented on, was introduced by #18. Happy to discuss anything related over there.

As I understood from the launchpad thread, this is marked Won't Fix for the ubuntu package only. It was fixed in the upstream according to this comment from over a year ago.

I could be wrong though. I did run the role and make sure it works. But as it is a race condition problem, I should've probably run this many times, which I didn't, so maybe I could run systemctl restart nginx a few times to make sure it works. To be sure, what version of nginx did you face this issue with?

nginx/1.18.0. The comment also seems to refer to the Ubuntu package, no? The role installs from Nginx directly. Did you spot a link to a commit/change in the thread, so we can see that the actual fix was?

The issue has been fixed https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864

Also: * Rename 'nginx configuration changed' handler to 'reload nginx'

This reverts commit 86b11d3. To be discussed and done in a separate PR

lucendio

One last change I'd like to ask for: move the Prometheus collector port 8443 into a role variable in defaults/main.yml. Otherwise, LGTM.

roles/sft-server/tasks/traffic.yml

lucendio · 2020-09-10T13:57:09Z

roles/sft-server/templates/metrics.vhost.conf.j2

+    listen 8443 ssl http2;
+    listen [::]:8443 ssl http2;


Move to a variable in defaults/main.yml.

Moved it to a constant. Let's make it overridable as we need to.

Thank you.

Moved it to a constant.

I'm the last one disagreeing with an extensive use of constants ;) I just had this line in the back of my head, which could be - when configured as part of the environment variables - feed into both, Terraform and the Ansible role. Anyway, later.

akshaymankar force-pushed the akshaymankar/node-exporter branch from 9fdd363 to a51b26b Compare August 31, 2020 10:16

akshaymankar marked this pull request as ready for review August 31, 2020 10:17

arianvp reviewed Aug 31, 2020

View reviewed changes

roles/sft-server/meta/main.yml Outdated Show resolved Hide resolved

akshaymankar added 2 commits August 31, 2020 14:48

Add node-exporter without any authentication

85c7830

Implement TLS based auth for metrics

618f3cb

* Add role to create CA and client certificates This role will likely only be used only for boostrapping an environment. It doesn't take care of certificate rotation yet, but it can be made to do so as and when required.

akshaymankar force-pushed the akshaymankar/node-exporter branch from a51b26b to 618f3cb Compare August 31, 2020 12:49

arianvp approved these changes Aug 31, 2020

View reviewed changes

lucendio reviewed Aug 31, 2020

View reviewed changes

akshaymankar added 10 commits September 1, 2020 10:10

Cleanup certs created for tests

480dac8

Use import_tasks instead of include

489d495

Document mandatory variable 'sft_metrics_ca_url'

6be9ee7

Remove documentation for obsolete 'metrics_fqdn'

Merge branch 'develop' into akshaymankar/node-exporter

6467c2d

Make roles useable by ansible-galaxy

7ac6129

Rename monitoring-certs -> sft-monotring-certs

7698563

Also: * comment out usage of use_common_name_for_san as it is not available in ansible-2.7

Use temporary dir for certs instead of files

c4df487

In ansible 2.7, when openssl_csr sees and empty file, it panics.

Left over rename

e2ad045

Name the imports in sft-monitoring-certs role

1c8c369

Omit 'not_after' fields in CA signing

dc05f01

They don't work so nicely with ansible 2.7, they need exact timestamp and calculating that is not fun. Fortunately they default to 10 years anyways.

lucendio reviewed Sep 3, 2020

View reviewed changes

roles/sft-monitoring-certs/tasks/main.yml Outdated Show resolved Hide resolved

roles/sft-monitoring-certs/tasks/ca-cert.yml Outdated Show resolved Hide resolved

akshaymankar added 5 commits September 7, 2020 16:55

Ensure all variables exist before creating certs

a778b06

Add doc about sft-monioring-certs role

6857ce2

Merge branch 'develop' into akshaymankar/node-exporter

cf9bd3f

Ensure at least one way of retrieving sftd artifact works

aba1eb8

Check for the correct sftd artifact file

5b5783a

arianvp reviewed Sep 8, 2020

View reviewed changes

akshaymankar added 5 commits September 8, 2020 13:58

Extract sft_metric_ca_path into a constant

bb73ceb

Fix wrong merge

75624fd

Add comment about CSR validity

f68e498

Remove unnecessary sleep from nginx

86b11d3

The issue has been fixed https://bugs.launchpad.net/ubuntu/+source/nginx/+bug/1581864

Add handler to reload systemd, use it when changing units

3efa542

Also: * Rename 'nginx configuration changed' handler to 'reload nginx'

Revert "Remove unnecessary sleep from nginx"

afca3a7

This reverts commit 86b11d3. To be discussed and done in a separate PR

lucendio approved these changes Sep 10, 2020

View reviewed changes

Extract nginx port numbers into constants

79d07f9

akshaymankar merged commit 3f4f804 into develop Sep 10, 2020

akshaymankar deleted the akshaymankar/node-exporter branch September 10, 2020 15:07

akshaymankar mentioned this pull request Sep 25, 2020

[role:sft-server] Copy list of dependencies into the top-level #21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node-exporter #13

Add node-exporter #13

akshaymankar commented Aug 28, 2020 •

edited

Loading

arianvp left a comment

lucendio left a comment

lucendio left a comment

arianvp Sep 8, 2020

arianvp Sep 8, 2020

akshaymankar Sep 8, 2020

lucendio Sep 8, 2020

akshaymankar Sep 9, 2020

lucendio Sep 9, 2020

lucendio left a comment

lucendio Sep 10, 2020

akshaymankar Sep 10, 2020

lucendio Sep 10, 2020

Add node-exporter #13

Add node-exporter #13

Conversation

akshaymankar commented Aug 28, 2020 • edited Loading

arianvp left a comment

Choose a reason for hiding this comment

lucendio left a comment

Choose a reason for hiding this comment

lucendio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucendio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaymankar commented Aug 28, 2020 •

edited

Loading