Add NGINX metrics and update security context

[Jeroen0494]

Pull Request

Description of the change

• Add additional securityContext options for the metrics exporter.
• Add the option for including NGINX metrics collection. The allow rules are for a default k3s installation and service just as an example.

Benefits

• securityContext makes my polaris dashboard (and kube-bench) happy.
• Metrics can now be collected properly.
• Metrics for NGINX are collected and automatically exported by the service monitor. You can use a Grafana dashboard to monitor connections.

Possible drawbacks

• None, it's optional

Applicable issues

• fixes Fix: extraVolumeMounts for Nginx Container #277

Checklist

• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Variables are documented in the README.md

[jessebot]

There are also conflicts with the deployment.yaml.

Added Kate and Taylor as reviewers since this is a larger PR and I want to be sure I don't miss anything. 🙏

[Jeroen0494]

Rebased on current master and fixed markdown table.

[Jeroen0494]

Uhm, I think my request for re-review removed @tvories and @provokateurin from the requested reviewers. Sorry.

[jessebot]

Looks like there's conflicts for the chart, values, and readme. The README is likely due to the me splitting up the config parameters so that they're more easily linkable. Sorry about that :( If you can resolve the conflicts, I can take a look at what Kate requested for changes and review the PR again.

[Jeroen0494]

Rebased on current master branch.

[jessebot]

approved lint and test run

[Jeroen0494]

@jessebot thanks for the review. The quotes for user and group numbers were my mistake. I also updated the securityContext, but didn't see the removed quoted from upstream.

[Jeroen0494]

I've rebased as best as possible.

[wrenix]

It looks nice, thank you for your work.

Do you like to write simething for a sidecar with an nginx-exporter and maybe adjust or add an ServiceMonitor for Prometheus?

nginx exporter:

• from nginx itself: https://github.com/nginxinc/nginx-prometheus-exporter
• use telegraf as exporter (input: nginx, output: prometheus_client)

[Jeroen0494]

It looks nice, thank you for your work.

Do you like to write simething for a sidecar with an nginx-exporter and maybe adjust or add an ServiceMonitor for Prometheus?

nginx exporter:

• from nginx itself: https://github.com/nginxinc/nginx-prometheus-exporter
• use telegraf as exporter (input: nginx, output: prometheus_client)

Prometheus already has their own Helm chart for this, I'd rather not reinvent the wheel. Maybe you could set this as a dependency when metrics is enabled, but for now I'd just like to have this included. This PR has been open for so long already.

[jessebot]

Added a few more notes as there's some defaults that have been set in the templates that should be configurable.

[jessebot]

Merged in the changes from #483

[Jeroen0494]

I've included your comments and squashed my commits to make everything more legible. Ready for another review.

[wrenix]

Still something todo (some things should be visible if you run chart-testing on your computer)

[wrenix]

This value has no affact anywhere ....
But maybe usefull (on deployment under ports and your ConfigMap rendered; or service).

[Jeroen0494]

This is actually used in nginx-config.yaml:

            {{- range .Values.metrics.nginx.allow }}
                allow {{ . }};
            {{- end }}

[jessebot]

.Values.metrics.nginx.allow is not the same as .Values.nginx.metrics.service.port, which is the values you have here. Am I maybe misunderstanding something?

[Jeroen0494]

Fixes included, I think.

[jessebot]

As @wrenix said, this could be with and it would save on required code, also you need {{}} surrounding the .Values.nextcloud.configs.defaultMode or the helm template won't work render:

Suggested change

	{{- if .Values.nextcloud.configs.defaultMode }}
	defaultMode: .Values.nextcloud.configs.defaultMode
	{{- end }}
	{{- with .Values.nextcloud.configs.defaultMode }}
	defaultMode: {{ . }}
	{{- end }}

You can commit this suggestion directly.

[jessebot]

echoing wrenix here since it won't let me make a suggestion in a comment reply to their comment.

Suggested change

	{{- if .Values.nextcloud.configs.defaultMode }}
	defaultMode: .Values.nextcloud.configs.defaultMode
	{{- with .Values.nextcloud.configs.defaultMode }}
	defaultMode: {{ . }}

You can commit this suggestion directly from this PR.

[jessebot]

Suggested change

	{{- if .Values.nextcloud.configs.defaultMode }}
	defaultMode: .Values.nextcloud.configs.defaultMode
	{{- with .Values.nextcloud.configs.defaultMode }}
	defaultMode: {{ . }}

[jessebot]

Requesting a few changes to make the templates render properly and reduce wordiness.

[jessebot]

I've added metrics.securityContext and metrics.podSecurityContext in #609, so this PR can be reduced in complexity to just include the nginx metrics changes. 🙏