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

GrafanaDashboard should support substitution from GrafanaDatasource name #1684

Open
gecube opened this issue Sep 22, 2024 · 3 comments
Open
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@gecube
Copy link

gecube commented Sep 22, 2024

Hi!

Like a DevOps I want to apply a bunch of Datasources against the cluster. Then I want to apply GrafanaDashboard using this datasources, but I don't want to play around the name of Datasources, rather just pointing to a Datasource namespace and name. All other work should be done by operator. I will try to explain on the example.

The current behaviour:

I am applying the datasources:

apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
  name: influxdb-v2
  namespace: tenant-root
spec:
  datasource:
    access: proxy
    editable: true
    isDefault: false
    jsonData:
      defaultBucket: ""
      httpMode: POST
      organization: myorg
      version: Flux
    name: InfluxDB-V2
    type: influxdb
    url: http://influxdb.tenant-test.svc:8086
  instanceSelector:
    matchLabels:
      dashboards: grafana
---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
  name: mysql-tst-icarus
  namespace: tenant-root
spec:
  datasource:
    access: proxy
    editable: true
    isDefault: false
    jsonData: {}
    name: tst_icarus
    type: mysql
    url: mysql.tenant-test.svc:3306
    user: root
    secureJsonData:
      password: root
    database: tst_icarus
  instanceSelector:
    matchLabels:
      dashboards: grafana

and setting their name in spec.datasource.name (it would be nice if it could be autogenerated).

Then I am applying dashboard:

apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDashboard
metadata:
  name: openstef-new
  namespace: tenant-root
spec:
  folder: openstef
  instanceSelector:
    matchLabels:
      dashboards: grafana
  url: https://raw.githubusercontent.com/OpenSTEF/openstef-reference/master/grafana-configurator/volumes/config/dashboards/station_forecasts.json
  datasources:
  - inputName: "000000005"
    datasourceName: "tst-icarus"
  - inputName: "vowSQsa4z"
    datasourceName: "InfluxDB-V2"
  resyncPeriod: 30

What is important here - the inputName is the name inside of dashboard and datasourceName what is known to current grafana instance. I want to emphasize that IT IS ERROR PRONE to paste the datasource name manually. So the desired spec should look like:

apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDashboard
metadata:
  name: openstef-new
  namespace: tenant-root
spec:
  folder: openstef
  instanceSelector:
    matchLabels:
      dashboards: grafana
  url: https://raw.githubusercontent.com/OpenSTEF/openstef-reference/master/grafana-configurator/volumes/config/dashboards/station_forecasts.json
  datasources:
  - inputName: "000000005"
    datasourceRefName: "mysql-tst-icarus"
    datasourceRefNamespace: "tenant-root"
  - inputName: "vowSQsa4z"
    datasourceRefNameName: "influxdb-v2"
# datasourceRefNamespace: "tenant-root" ### Could be omited - then use current one.
  resyncPeriod: 30
@gecube gecube added enhancement New feature or request needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 22, 2024
@theSuess
Copy link
Member

We discussed this issue in our weekly sync meeting and in our opinion, this does not fix the issue of it being error-prone to paste the data source name. Instead of pasting the name, you are now pasting the resource name & namespace (which is more opportunity to introduce errors).

We do not maintain an admission webhook that could prevent invalid manifests from being created, so the best you'd get is an error in the GrafanaDashboard resource.

Does this prevent you from using the Grafana Operator or is it mostly a usability issue?

@gecube
Copy link
Author

gecube commented Sep 23, 2024

@theSuess Hi! Thanks for interest in the issue. As I see there is two workflow:

  • we already have some grafana with preconfigured datasources and user wants to upload some dashboards to it from k8s cluster with gitops approach. Then it is logical to leave the option to set the name of datasource.
  • we have a ephemeral grafana instance (like in kube prometheus stack) and this instance would be populated with all datasources and dashboards from scratch. In this case I don't care about datasources and dashboards internal name but dashboards should be somehow organised into catalogues and have a meaningful human readable name (like "Ingress Nginx Metrics Dashboard"). For that case it would be nice to have refs. Also I want to mention that many operators support referencing already precreated entities - like https://github.com/aws-controllers-k8s or FluxCD with it's HelmRelease when we can refer to already uploaded HelmChart to the cluster or reference it by name and HelmRepository. So I don't see here any logical mistake. Just attempt to make life of DevOps easier.

Does this prevent you from using the Grafana Operator or is it mostly a usability issue?

It is another drop that stops me from using Grafana Operator as I don't feel it mature enough. I am investigating it's advantages and possibilities and to be honest - I see a great potential, so I'd like to have a turn-key solution for managing grafana instances. Also I want to mention that main purpose of operator (from my POV) would be serving single instance grafana configuration, rather than supporting multiple grafana instances, but we are considering the latter because of the lack of user access controls in open source version of grafana.

@theSuess
Copy link
Member

theSuess commented Oct 7, 2024

Thanks for the elaboration, I now see where you're coming from.

We'll soon implement configurable UIDs for all resources, including data sources. In my opinion, the cleaner solution would then be to use the UID for the data source mapping field. As an example:

apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDatasource
metadata:
  name: example-grafanadatasource
spec:
  datasource:
    # ...
  # EXAMPLE: NOT YET IMPLEMNTED
  uid: tenant-root
---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDashboard
metadata:
  name: openstef-new
  namespace: tenant-root
spec:
  url: https://raw.githubusercontent.com/OpenSTEF/openstef-reference/master/grafana-configurator/volumes/config/dashboards/station_forecasts.json
  datasources:
  - inputName: "000000005"
    # EXAMPLE: NOT YET IMPLEMNTED
    datasourceUID: 'mysql-tst-icarus'

Do you think this would offer the same affordances in your use case?

Once the ability to set UIDs has finalized, implementing references also becomes much easier but if relying on UIDs alone is also a valid choice for you, keeping the complexity low would be our main goal

@theSuess theSuess added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

2 participants