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

x509-certificate-exporter: strange fixes #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kruftik
Copy link

@kruftik kruftik commented Jun 6, 2021

  • 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 }}

@npdgm npdgm self-requested a review August 11, 2021 16:14
@npdgm
Copy link
Member

npdgm commented Aug 11, 2021

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

@npdgm npdgm self-assigned this Aug 11, 2021
@npdgm npdgm changed the title strange fixes x509-certificate-exporter: strange fixes Aug 11, 2021
@npdgm
Copy link
Member

npdgm commented Aug 11, 2021

@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
Copy link
Author

kruftik commented Aug 18, 2021

@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
Copy link
Author

kruftik commented Oct 22, 2021

@npdgm,

does the PR look good for merge? :)

@kruftik
Copy link
Author

kruftik commented Dec 26, 2021

@npdgm ,

this PR is still worth to be reviewed!

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.

2 participants