helm-charts icon indicating copy to clipboard operation
helm-charts copied to clipboard

x509-certificate-exporter: strange fixes

Open kruftik opened this issue 3 years ago • 5 comments

  • in order not to see double slashes in rendered manifests, helm trimPrefix function added in directory-to-file concats
  • to distinguish the generated daemonsets without requiring a user to add separate custom labels in a values file, the following label added to the generated daemonsets, podtemplates of the daemonsets and corresponding selector rules:
x509-certificate-exporter.enix.io/daemonset-name: {{ $dsName | quote }}

kruftik avatar Jun 06 '21 12:06 kruftik

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

npdgm avatar Aug 11 '21 16:08 npdgm

@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

npdgm avatar Aug 11 '21 17:08 npdgm

@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 avatar Aug 18 '21 03:08 kruftik

@npdgm,

does the PR look good for merge? :)

kruftik avatar Oct 22 '21 05:10 kruftik

@npdgm ,

this PR is still worth to be reviewed!

kruftik avatar Dec 26 '21 05:12 kruftik