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

feat(x509-certificate-exporter): add hostNetwork mode support #49

Closed vi7 closed 3 years ago

vi7 commented 3 years ago

hey @Zempashi @npdgm

one more PR, adding hostNetwork mode support and making Service truly "headless"

Let me shortly describe my need so you'll understand the reason of the PRs.

We have a standalone baremetal Prometheus deployment, i.e. outside of the K8S cluster (also baremetal) on a separate host. Thus initially, in order to reach the x509 cert exporter, I've added a nodePort Service support in my previous PR and then realized that Prometheus must collect exporter data via endpoints K8S SD rather than service SD which was solved by adding hostNetwork mode support, ending up with the setup pretty much similar to the node_exporter, hence this PR.

Also I've made Service really headless as per K8S docs, because in the end we do not need a ClusterIP allocation for it.

Couple of questions assuming the above:

npdgm commented 3 years ago

Hey @vi7 Thanks for the feedback, your deployment is not uncommon and that's definitely a case we want to address!

Please disregard I had put approval on this PR, you're correct the upgrade conflict is an issue.

I'm having doubts regarding this Service now. It's true it should have been headless from the beginning, it was only intended for k8s endpoint discovery as you pointed out. And actually we should be providing a PodMonitor instead of a ServiceMonitor. This will be changed soon. Still the Service may come useful to someone especially when using the Secrets exporter only which is a single Pod. So yeah... I guess we could drop the nodePort feature, or let this service target the Deployment only and not DaemonSets.

vi7 commented 3 years ago

@npdgm thank you for the comment!

I don't have a right setup atm to test separate Service for the Deployment thus I've just dropped the nodePort support.

What would be the final decision on the headless Service? Should I remove clusterIP: None from the PR?

npdgm commented 3 years ago

I don't have a right setup atm to test separate Service for the Deployment thus I've just dropped the nodePort support.

Looks good. I will handle the ServiceMonitor to PodMonitor transition outside this PR and add options for optional services for each exporter type.

What would be the final decision on the headless Service? Should I remove clusterIP: None from the PR?

No let's make it headless for real :+1: Other Services will be offered for non-SD usages. The trick is changing the service name and postfix with -headless, it's less disruptive than forced upgrades for about everyone. Making the changes now so we can release hostNetwork!

vi7 commented 3 years ago

@npdgm awesome, thank you!