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

[kube-prometheus-stack] Grafana crd datasources #5240

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dzirg44
Copy link

@dzirg44 dzirg44 commented Jan 27, 2025

Introducing Grafana Datasources as CRDs

Special notes for your reviewer

this PR is a result of #5216
I essence I don't expect any problems but you have to pay attention to 3 things

  1. due to the helper usage the sort order in the result yaml config map is broken, I don't think it is a big deal, but let me know if you have any other thoughts
Details

data:
  datasource.yaml: |-
    apiVersion: 1
    datasources:
      - access: proxy
        isDefault: true
        jsonData:
          httpMethod: POST
          timeInterval: 30s
          timeout: null
        name: Prometheus
        type: prometheus
        uid: prometheus
        url: http://prometheus-stack-kube-prom-prometheus.default:9090/
      - access: proxy
        isDefault: false
        jsonData:
          handleGrafanaManagedAlerts: false
          implementation: prometheus
        name: Alertmanager
        type: alertmanager
        uid: alertmanager
        url: http://prometheus-stack-kube-prom-alertmanager.default:9093/

  1. the name of name: {{ include "kube-prometheus-stack.fullname" $ }}-{{ include "kube-prometheus-stack.grafana.sanitizeName" $ds.name }} , probably you have a better idea or conventions to name it.

  2. default values for

      app.kubernetes.io/instance: grafana
      app.kubernetes.io/name: grafana-operator

I have them in 2 places (values, and the resource), but I often do like this in my charts, but idk if it is appropriate here.
@jkroepke
Also, if I have to mention about it in the documentation - let me know.

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Copy link
Member

@jkroepke jkroepke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for split this help. It help at least me to review this with a bit more focus.

To go through the files and leave comments that I have in my mind.

charts/kube-prometheus-stack/values.yaml Outdated Show resolved Hide resolved
charts/kube-prometheus-stack/values.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,27 @@
{{- if or (and .Values.grafana.enabled .Values.grafana.sidecar.datasources.enabled) .Values.grafana.forceDeployDatasources }}
{{- if .Values.grafana.operatorDatasources }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be merged into the first condition?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is something that I would like to discuss, I know that probably it is opinionated , but this code

{{- if and (or (and .Values.grafana.enabled .Values.grafana.sidecar.datasources.enabled) .Values.grafana.forceDeployDatasources) .Values.grafana.operatorDatasources }}
{{- $datasources := fromYaml (include "kube-prometheus-stack.grafana.datasources" .) }}
{{- range $ds := $datasources.datasources }}
---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
  name: {{ include "kube-prometheus-stack.fullname" $ }}-{{ include "kube-prometheus-stack.grafana.sanitizeName" $ds.name }}
  namespace: {{ template "kube-prometheus-stack-grafana.namespace" $ }}
  labels:
    app: {{ template "kube-prometheus-stack.name" $ }}-grafana
    {{- include "kube-prometheus-stack.labels" $ | nindent 4 }}
spec:
  instanceSelector:
    matchLabels:
      {{- toYaml $.Values.grafana.operatorInstanceSelector | nindent 6 }}
  datasource:
    {{- $ds | toYaml | nindent 4 }}
{{- end }}
{{- end }}

reads harder than

{{/* Define helper variables for complex conditions */}}
{{- $grafanaEnabled := and .Values.grafana.enabled .Values.grafana.sidecar.datasources.enabled }}
{{- $shouldDeploy := or $grafanaEnabled .Values.grafana.forceDeployDatasources }}

{{/* Only proceed if all required conditions are met */}}
{{- if $shouldDeploy }}
{{- if .Values.grafana.operatorDatasources }}

{{/* Process datasources */}}
{{- $datasources := fromYaml (include "kube-prometheus-stack.grafana.datasources" .) }}
{{- range $ds := $datasources.datasources }}
---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
  name: {{ include "kube-prometheus-stack.fullname" $ }}-{{ include "kube-prometheus-stack.grafana.sanitizeName" $ds.name }}
  namespace: {{ template "kube-prometheus-stack-grafana.namespace" $ }}
  labels:
    app: {{ template "kube-prometheus-stack.name" $ }}-grafana
    {{- include "kube-prometheus-stack.labels" $ | nindent 4 }}

spec:
  {{/* Instance selector configuration */}}
  instanceSelector:
    matchLabels:
      {{- toYaml $.Values.grafana.operatorInstanceSelector | nindent 6 }}
  
  {{/* Datasource configuration */}}
  datasource:
    {{- $ds | toYaml | nindent 4 }}

{{- end }}{{/* end range datasources */}}
{{- end }}{{/* end if .Values.grafana.operatorDatasources */}}
{{- end }}{{/* end if $shouldDeploy */}}

Comments I took from my charts so just pay attention to the code.
@jkroepke

@@ -1,84 +1,28 @@
{{- if or (and .Values.grafana.enabled .Values.grafana.sidecar.datasources.enabled) .Values.grafana.forceDeployDatasources }}
{{- if .Values.grafana.configMapDatasources }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be merged into line 1 condition

{{- $datasources := list -}}
{{- if .Values.grafana.sidecar.datasources.defaultDatasourceEnabled }}
{{/* Create jsonData dictionary first */}}
{{- $jsonData := dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting idea to use the dict + set function to build an structure, but not sure about maintainability.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep indeed, when I look at it after 1 week, can't say for sure what it does , I would like to write tests for these functions but helm is pretty limited in this sphere(

Comment on lines +17 to +29
{{- $defaultDS := dict -}}
{{- $_ := set $defaultDS "name" .Values.grafana.sidecar.datasources.name -}}
{{- $_ := set $defaultDS "type" "prometheus" -}}
{{- $_ := set $defaultDS "uid" .Values.grafana.sidecar.datasources.uid -}}
{{- $_ := set $defaultDS "url" (default (printf "http://%s-prometheus.%s:%v/%s"
(include "kube-prometheus-stack.fullname" .)
(include "kube-prometheus-stack.namespace" .)
.Values.prometheus.service.port
(trimPrefix "/" .Values.prometheus.prometheusSpec.routePrefix)
) .Values.grafana.sidecar.datasources.url) -}}
{{- $_ := set $defaultDS "access" "proxy" -}}
{{- $_ := set $defaultDS "isDefault" .Values.grafana.sidecar.datasources.isDefaultDatasource -}}
{{- $_ := set $defaultDS "jsonData" $jsonData -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a suggestion which can be considered as alternative. It's just an option to discuss about if possible and it's not indented as "please use this".

Suggested change
{{- $defaultDS := dict -}}
{{- $_ := set $defaultDS "name" .Values.grafana.sidecar.datasources.name -}}
{{- $_ := set $defaultDS "type" "prometheus" -}}
{{- $_ := set $defaultDS "uid" .Values.grafana.sidecar.datasources.uid -}}
{{- $_ := set $defaultDS "url" (default (printf "http://%s-prometheus.%s:%v/%s"
(include "kube-prometheus-stack.fullname" .)
(include "kube-prometheus-stack.namespace" .)
.Values.prometheus.service.port
(trimPrefix "/" .Values.prometheus.prometheusSpec.routePrefix)
) .Values.grafana.sidecar.datasources.url) -}}
{{- $_ := set $defaultDS "access" "proxy" -}}
{{- $_ := set $defaultDS "isDefault" .Values.grafana.sidecar.datasources.isDefaultDatasource -}}
{{- $_ := set $defaultDS "jsonData" $jsonData -}}
{{- $defaultDS := (dict
"name" .Values.grafana.sidecar.datasources.name
"type" "prometheus"
"uid" .Values.grafana.sidecar.datasources.uid
"url" (default (printf "http://%s-prometheus.%s:%v/%s"
(include "kube-prometheus-stack.fullname" .)
(include "kube-prometheus-stack.namespace" .)
.Values.prometheus.service.port
(trimPrefix "/" .Values.prometheus.prometheusSpec.routePrefix)
) .Values.grafana.sidecar.datasources.url)
"access" "proxy"
"isDefault" .Values.grafana.sidecar.datasources.isDefaultDatasource
"jsonData" $jsonData
) -}}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkroepke the main problem here, that if we take your approach we loose the possibility to add comments, because any comments between dict keys break the formatting. At the beginning I implemented it in a way you suggested but later I realized that I cannot make nice conditionals with comments, so I switched to this version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants