add grafana dashboards to the chart

[bergerx]

We'd like to have the grafana dashboards to be installed as part of the helm chart.

The grafana chart already supports defining the dashboards as configmaps using a sidecar (which is also used part of the kube-prometheus-stack chart by default), its described here:
https://github.com/grafana/helm-charts/tree/1f9f9fdf8be5fff63663d121d71dbeaa120ca6ca/charts/grafana#sidecar-for-dashboards

Creation of the grafana dashboards could be enabled based on some flags/configuration in the values file.
Here is an example implementation: https://github.com/helm/charts/blob/0c093133575d640710959d3442d5bad59c776942/stable/sealed-secrets/templates/configmap-dashboards.yaml

I'll try to put together a PR but in the meantime if someone else wants to work on it here is a sample implementation based on the example above

Add these into values.yaml

dashboards:
  # If enabled, ingress-nginx will create a configmap with a dashboard in json that's going to be picked up by grafana
  # See https://github.com/grafana/helm-charts/tree/main/charts/grafana#configuration - `sidecar.dashboards.enabled`
  create: false
  # Extra labels to apply to the dashboard configmaps
  labels:
  # The namespace where the dashboards are deployed, defaults to the installation namespace
  namespace:

And add these to

{{- if .Values.dashboards.create }}
{{- $namespace := .Values.dashboards.namespace | default $.Release.Namespace }}
{{- range $path, $_ :=  .Files.Glob  "../../deploy/grafana/dashboards/*.json" }}
{{- $filename := trimSuffix (ext $path) (base $path) }}
apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ template "ingress-nginx.fullname" $ }}-{{ $filename }}
  namespace: {{ $namespace }}
  labels:
    grafana_dashboard: "1"
    {{- include "ingress-nginx.labels" . | nindent 4 }}
    {{- if $.Values.dashboards.labels }}
    {{- toYaml $.Values.dashboards.labels | nindent 4 }}
    {{- end }}
data:
  {{ base $path }}: |-
{{ $.Files.Get $path | indent 4 }}
---
{{- end }}
{{- end }}

/kind feature

[bergerx]

If this seems easy enough to the maintainers, maybe we can mark this as a 'good first issue'?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[bergerx]

/remove-lifecycle stale

[bergerx]

@aledbf, do you mind tagging this as a good-first-issue. I believe there is sufficient amount of information in the description already. Maybe someone picks this up before we revisit this to convert into a pr.

[VengefulAncient]

This would be really nice to have. IMO, kustomize is hard to maintain and, ironically, customize, compared to Helm charts. (The used approach is also quite outdated, no one wants to deal with NodePorts and kubectl port-forward when ingress exists.)

[antoineozenne]

It could be great!

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[MattJeanes]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[antoineozenne]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[antoineozenne]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[antoineozenne]

/remove-lifecycle stale

[longwuyuan]

some work was done recently. Please check docs

[antoineozenne]

some work was done recently. Please check docs

I don't see it. Can you please attach a link?

[longwuyuan]

https://kubernetes.github.io/ingress-nginx/user-guide/monitoring/

…

On 06-Mar-2022, at 9:04 PM, Antoine ***@***.***> wrote: some work was done recently. Please check docs I don't see it. Can you please attach a link? — Reply to this email directly, view it on GitHub <#6381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWSA6IT6F2OPHRBUZC3U6TGBHANCNFSM4TBYLNRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.

[VengefulAncient]

This is a good start but the original issue stands. The entire Prometheus/Grafana stack with these dashboards needs to be made available as a configurable part of the ingress-nginx Helm chart so it can be properly maintained by devops teams that implement ingress-nginx in their clusters. A manual deployment with kustomize is not a suitable solution. Therefore, this issue should be kept open.

[longwuyuan]

You are absolutely right. All your questions are valid and your suggestion is also valid. But it can not be done because of best-practices. First there is lack of dev resources so radically modifying the helm-chart is out of the question. It can not be maintained if we introduce all the monitoring software into the helm chart. Secondly, even though you have problems, the best-practice for dashboards is to publish them at https://grafana.com/grafana/dashboards/ <https://grafana.com/grafana/dashboards/> . The project can publish dashboard there but I think other people already did that. Point being the current availability of dev resources does not allow for maintaining a curated solution like you mentioned. Thanks, :Long

…

On 06-Mar-2022, at 10:27 PM, VengefulAncient ***@***.***> wrote: This is a good start but the original issue stands. The entire Prometheus/Grafana stack with these dashboards needs to be made available as configurable part of the ingress-nginx Helm chart so it can be properly maintained by devops teams that implement ingress-nginx in their clusters. A manual deployment with kustomize is not a suitable solution. Therefore, this issue should be kept open. — Reply to this email directly, view it on GitHub <#6381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWQQTBMA3QB2PCYVFVDU6TPY3ANCNFSM4TBYLNRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.

[bergerx]

@longwuyuan I see that in the Prometheus and Grafana installation doc, the dashboard json is already hosted in the repo and maintained as part of the ingress-nginx repo already.

Given that the dashboard source code is already part of the repo (which would be the main body that would require significant maintenance overhead over time), delivery of this feature would be adding a couple of manifest files in the chart to include that existing dashboard json file into the released chart. After a couple of PRs to get some more parts parameterized, I'd personally expect this to be one of the most stable parts of the chart given that the maintenance of teh dashboard is already out of the scope of this delivery.

[longwuyuan]

PRs are welcome as the project is short on dev time. But I am not clear on several aspects. One significant one aspect being, the dashboard JSON/SourceCode is relevant to the Grafana chart and not the ingress-nginx controller chart. I think you are proposing that we ship a grafana-chart-dashboard-json file in the project’s helm-chart releases, that will hopefully be extracted and used in a completely different helm-chart-release, that is external to the ingress-nginx helm-chart release. And if it not used, then it just sits there in the tree of the ingress-nginx controller. I think your proposition solves the use-case you have described, but it makes the ingress-nginx chart ugly. I don’t recommend that. Maybe you can take this dashboard and fork your own Grafana-Chart that includes this dashboard. Lets see what others comment. Thanks, ; Long Wu Yuan

…

On 07-Mar-2022, at 8:40 PM, Bekir Dogan ***@***.***> wrote: @longwuyuan <https://github.com/longwuyuan> I see that in the Prometheus and Grafana installation <https://kubernetes.github.io/ingress-nginx/user-guide/monitoring/#prometheus-and-grafana-installation> doc, the dashboard json <https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/grafana/dashboards/nginx.json> is already hosted in the repo and maintained as part of the ingress-nginx repo already. Given that the dashboard source code is already part of the repo (which would be the main body that would require significant maintenance overhead over time), delivery of this feature would be adding a couple of manifest files in the chart to include that existing dashboard json file into the released chart. After a couple of PRs to get some more parts parameterized, I'd personally expect this to be one of the most stable parts of the chart given that the maintenance of teh dashboard is already out of the scope of this delivery. — Reply to this email directly, view it on GitHub <#6381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWTHXW4TZZVBZTIIFELU6YL4TANCNFSM4TBYLNRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[kfox1111]

helm supports conditionals. Can just wrap the grafana dashboard in a conditional to enable it if the user wishes in the ingress-nginx chart. No need for a separate chart.

[longwuyuan]

I still feel PRs are welcome. But a conditional or not conditional does not make sense to me because the ingress-nginx controller does not bundle the Grafana software so what is conditional “enablement” here ! Thanks, ; Long Wu Yuan

…

On 09-Mar-2022, at 10:21 PM, kfox1111 ***@***.***> wrote: helm supports conditionals. Can just wrap the grafana dashboard in a conditional to enable it if the user wishes in the ingress-nginx chart. No need for a separate chart. — Reply to this email directly, view it on GitHub <#6381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWRFAUTXEHTQEHE6LGTU7DJITANCNFSM4TBYLNRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[MattJeanes]

My understanding is that enabling it would simply put a configmap on the cluster with special labels/annotations which is picked up by a special grafana sidecar container already on the cluster and automatically added into grafana. You can see an example of this here: https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/templates/grafana/dashboards-1.14/grafana-overview.yaml

[longwuyuan]

Ok. I did not read that yet. Where is the sidecar. In the ingress-controller pod ? Thanks, ; Long Wu Yuan

…

On 10-Mar-2022, at 2:19 AM, Matt Jeanes ***@***.***> wrote: My understanding is that enabling it would simply put a configmap on the cluster with special labels/annotations which is picked up by a special grafana sidecar container already on the cluster and automatically added into grafana. You can see an example of this here: https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/templates/grafana/dashboards-1.14/grafana-overview.yaml <https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/templates/grafana/dashboards-1.14/grafana-overview.yaml> — Reply to this email directly, view it on GitHub <#6381 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWXTOARTXMOHNR3FZHTU7EFFRANCNFSM4TBYLNRQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.

[bergerx]

The ingress-nginx chart already has a kind: ServiceMonitor manifest, that can only be used if a prometheus-operator installed and configured to watch that manifest. That manifest tells prometheus-operator to configure prometheus how/where to scrape ingress-nginx metrics.

The proposed manifest here would be a similar addition with some minor differences. This one will tell the grafana chart (which has the mentioned sidecar) to configure the ingress-nginx grafana dashboard. Prometheus-operator decided to use CRDs to pass prometheus configuration but in grafana's case they just picked ConfigMap with a special label over creating a new CRD.

Also most of the helm users deploy prometheus-operator through the kube-prometheus-stack chart, that already depends on the grafana chart that already has the mentioned sidecar that watches such dashboard ConfigMaps.

So, adding support for the dashboard in form of the ConfigMap would be aligned with supporting the kind: ServiceMonitor. In the same way people don't want to deploy the ServiceMonitor manifest by other means, they dont want to do it to deploy the dashboard.

If your concern is not to maintain the dashboard's json as part of the ingress-nginx, unfortunately its already in the repo and not to be added/copied as part of this issue/pr. This PR would just use the existing dashboard json thats already there.

I guess, at this point discussing these on a PR would be more productive for everyone to get the big picture better, I just didn't yet have time to do that yet. And if someone else does that in the meantime i'd really help all.

[longwuyuan]

@bergerx I agree. Makes sense. Requirements are that it has to be optional with default as opt-out. I would love to try this but I have not done it before so if nobody tries for a long time, and I get time, then I will try.

[longwuyuan]

/assign
Sorry for delay on this. I will try to work on this this week.

[longwuyuan]

@bergerx I started looking at this and my thoughts are as follows ;

• Including pieces for observability has to be optional and not a default opt-in. Because of this bundling the Grafana chart json in a default install of the ingress-nginx-controller chart is not my choice. Others may differ
• Just as controller.metrics.enabled is an optional key, using an optional key like controller.metrics.grafana.dashboard.enabled seems like the expected direction this should take
• This requires a lot of work, specially on testing, so you can submit a PR

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[MattJeanes]

I believe this is still a valuable addition to the chart

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[antoineozenne]

/remove-lifecycle stale

[rikatz]

Is there a PR on it?
We should probably have helm chart running independently of main ingress code, but remember that also we had recent complains that our helm charts are getting too complicated to maintain.

I'm not against a PR for this, honestly, but I would like to:

• Have the configmap being generated during helm release, and not copied/pasted as a configmap generating one more huge configmap file template on our chart
• Have conditionals and disable by default
• As we use Prometheus Operator manifests (we shouldn't IMO, but it is what it is) we should try to stick with this approach

[tao12345666333]

We can create a new child helm chart for this one.
If users want to enable this child helm chart, then it will generate the configmap includes all Grafana dashboard.

[rikatz]

/close

We are trying not to overload (more) helm charts. I would love to have some community maintained helm chart for grafana dashboards, but right now I think this is a burden we cannot maintain on the project.

[k8s-ci-robot]

@rikatz: Closing this issue.

In response to this:

/close

We are trying not to overload (more) helm charts. I would love to have some community maintained helm chart for grafana dashboards, but right now I think this is a burden we cannot maintain on the project.

Instructions 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/test-infra repository.