[kube-prometheus-stack] ConfigReloader ServiceMonitor HTTP scheme is not set correctly when HTTPS is enabled #3815
Conversation
Set HTTP as default scheme (only HTTP is supported by ConfigReloader) Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
Force HTTP for ConfigReloader scheme Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
n1kofr
requested review from
andrewgkew,
gianrubio,
gkarthiks,
GMartinez-Sisti,
QuentinBisson,
scottrigby and
Xtigyro
as code owners
Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
|
Looking over the file changes, doesn't this PR just hard-code the use of HTTP for all of the metric endpoints including for Alertmanager rather only impacting the metric endpoint for the config-reloader? |
|
The goal is to force the use of HTTP only for ConfigReloader port (for both Prometheus and Alertmanager) as I did not see any support for HTTPS on ConfigReloader and I would like to unlock the current issue which is impacted us (generating alerts). To summarize, collect metrics on Prometheus and Alertmanager is still using HTTPS, but ConfigReloader for both will be using HTTP. |
|
Sorry, I should have expanded the code around the change to get a better idea of the context. Thanks for clarifying that @n1kofr . |
There was a problem hiding this comment.
It appears there are no TLS options available to configure on config-reloader indeed - https://github.com/prometheus-operator/prometheus-operator/blob/52d1e55af2d223474aab35bf54bd56c2b9b22385/cmd/prometheus-config-reloader/main.go#L200C11-L200C11
|
Please fix conflicts so we can proceed 🙂 |
|
Ok, i will update the conflict right now :) |
Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
|
I fixed the conflicts, let me know if anything else needs to be done (or if i messed up somewhere) :) |
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [kube-prometheus-stack](https://github.com/prometheus-operator/kube-prometheus) ([source](https://github.com/prometheus-community/helm-charts)) | patch | `51.9.0` -> `51.9.1` | --- > ⚠ **Warning** > > Some dependencies could not be looked up. Check the warning logs for more information. --- ### Release Notes <details> <summary>prometheus-community/helm-charts (kube-prometheus-stack)</summary> ### [`v51.9.1`](https://github.com/prometheus-community/helm-charts/releases/tag/kube-prometheus-stack-51.9.1) [Compare Source](prometheus-community/helm-charts@kube-prometheus-stack-51.9.0...kube-prometheus-stack-51.9.1) kube-prometheus-stack collects Kubernetes manifests, Grafana dashboards, and Prometheus rules combined with documentation and scripts to provide easy to operate end-to-end Kubernetes cluster monitoring with Prometheus using the Prometheus Operator. #### What's Changed - \[kube-prometheus-stack] ConfigReloader ServiceMonitor HTTP scheme is not set correctly when HTTPS is enabled by [@​n1kofr](https://github.com/n1kofr) in prometheus-community/helm-charts#3815 #### New Contributors - [@​n1kofr](https://github.com/n1kofr) made their first contribution in prometheus-community/helm-charts#3815 **Full Changelog**: prometheus-community/helm-charts@prometheus-conntrack-stats-exporter-0.5.8...kube-prometheus-stack-51.9.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuMjQuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Reviewed-on: https://git.home/nrdufour/home-ops/pulls/159 Co-authored-by: Renovate <renovate@ptinem.io> Co-committed-by: Renovate <renovate@ptinem.io>
|
This change completely breaks working ServiceMonitor configs with Istio |
Hi @verejoel, can you please report an issue with the details so we can look into it? |
|
Sure no problem! Voila: #4039 |
…not set correctly when HTTPS is enabled (prometheus-community#3815) * Update servicemonitor.yaml Set HTTP as default scheme (only HTTP is supported by ConfigReloader) Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com> * Update servicemonitor.yaml Force HTTP for ConfigReloader scheme Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com> * Update Chart Version Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com> --------- Signed-off-by: Nicolas <60761885+n1kofr@users.noreply.github.com>
What this PR does / why we need it
The goal of this PR is to fix Prometheus target not being able to be scraped because of incompatible scheme for ConfigReloader when HTTPS is enabled on Prometheus.
@andrewgkew
@gianrubio
@gkarthiks
@GMartinez-Sisti
@scottrigby
@Xtigyro
@QuentinBisson
related to issue [https://github.com//issues/3802]
Which issue this PR fixes
Special notes for your reviewer
Checklist
[prometheus-couchdb-exporter])