enix / helm-charts

A collection of Helm packages brought to you by Enix Monkeys :monkey_face:
https://charts.enix.io
Apache License 2.0
56 stars 19 forks source link

x509-certificate-exporter: strange fixes #64

Open kruftik opened 3 years ago

kruftik commented 3 years ago
x509-certificate-exporter.enix.io/daemonset-name: {{ $dsName | quote }}
npdgm commented 3 years ago

Hi @kruftik, I'm so sorry this PR went unnoticed till now. We'll have a look to your contribution shortly! Cheers

npdgm commented 3 years ago

@kruftik, would you mind giving more information on how the daemonset-name label is to be used? Is that in conjunction with the Prometheus operator or will that be an other metrics stack? It's quite interesting having a way to distinguish which DS issued a metric, however I fail to see where that label would be matched or showed in a typical Prometheus operator setup. Unless this would be for additional PodMonitors matching each DS Pods, in which case the job label will identify the source. I'm asking so that perhaps we can push the implementation even further or add a documentation for this new feature. Thanks

kruftik commented 3 years ago

@npdgm ,

There are two main ideas behind the PR:

First of all, the chart currently has a bug: if we define multiple DaemonSet-s in the corresponding section of values.yaml file with different node selectors or some other differences, e.g. daemonset which are going to be run on master and worker kube-nodes: image

then generated manifests contain too generic label and label selector rules. The main problem they do not contain any daemonset-specific labels and entries in label selectors: image

, so that daemonset controller cannot distinguish pods generated by different daemonsets from each other. Such an issue results in unintended pod restarts during helm upgrade operation and other difficulties.

The PR is trying to mitigate the bug by adding to podSpec labels and labelSelector maps an additional label x509-certificate-exporter.enix.io/daemonset-name with daemonset name (the key in daemonSets values.yaml map) as a value.

Another problem of the chart is too straightforward path building logic in hostPath volume and volumeMount sections: it simply concatenates some base path, slash as a path separator and a path provided by values.yaml config: /mnt/watch/dir-{{ . | clean | sha1sum }}/{{ . | clean }}

Such simplicity results in slash doubling in the resulted string: /mnt/watch/kube-ee9e4e203c758bb95ec439d60c16fb4e8854efaf//etc/kubernetes, for instance.

The proposed change eliminates them by using trimPrefix function in {{ . | clean }} template piece.

kruftik commented 3 years ago

@npdgm,

does the PR look good for merge? :)

kruftik commented 2 years ago

@npdgm ,

this PR is still worth to be reviewed!