feat(chart): add prometheus servicemonitor support - #456
Conversation
kubernetes-prow
Bot
added
the
kind/feature
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ayush4958 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @Ayush4958. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/L
|
Hi @ajaysundark , thanks ... |
rawadhossain
left a comment
rawadhossain
left a comment
There was a problem hiding this comment.
Took a first look at this.
one question on the secure metrics setup, how is the prometheus serviceaccount supposed to get the metrics-reader permission?
also, looks like the new serviceMonitor values are missing from the README.
| {{- if .Values.metrics.secure }} | ||
| scheme: https | ||
| tlsConfig: | ||
| insecureSkipVerify: true |
There was a problem hiding this comment.
when metrics.secure is enabled, don't we also need to configure the bearer token for the metrics endpoint? also, can we avoid insecureSkipVerify here and use the generated cert instead?
There was a problem hiding this comment.
nice catch on token, I will add that.
for insecureSkipVerify, since prometheus is usually in a different namespace, it can't easily mount the cert manager secret natively.
instead of hardcoding, I will expose empty tlsConfig: {} block in values.yaml so users can configure it however they need
| {{- if not .Values.metrics.enabled }} | ||
| {{- fail "You enabled metrics.serviceMonitor.enabled, but metrics.enabled is false. Please enable metrics to use the ServiceMonitor." }} | ||
| {{- end }} | ||
| {{- if .Capabilities.APIVersions.Has "monitoring.coreos.com/v1" }} |
There was a problem hiding this comment.
if serviceMonitor.enabled=true but the CRD isn't installed, this just renders nothing. do we want to warn here, or is the current behavior intentional?
There was a problem hiding this comment.
it was intentional but I can see its a bad UX so I will add a warning to the chart's NOTES.txt
about crd not being installed
| # Note: testing .Capabilities.APIVersions.Has is not fully supported by helm-unittest, | ||
| # but assuming the environment supports it, we can test standard rendering. |
There was a problem hiding this comment.
I think helm-unittest supports capabilities.apiVersions here. Since it's already been used below, can we also add a case where CRD isn't present?
There was a problem hiding this comment.
yup, good catch I will add test case for it
for rbac, we don't know the user prometheus namespace, its okay for cluster admin to manually bind the metric reader role. |
|
hi @rawadhossain cc: @AvineshTripathi @ajaysundark thanks ... |
3 participants
Description
Adds native Prometheus Operator
ServiceMonitorsupport to the Helm chart.The template is safely guarded by
.Capabilities.APIVersions.Hasto prevent crashing non prometheus clusters & automatically handles TLS injection for secure metric endpoints.Related Issue
Fixes #443
Type of Change
/kind feature
Testing
helm templatetesting various edge cases (secure vs insecure metrics).Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?