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?
| {{- 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?
| # 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?
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?