Implement spec.uid for GrafanaDashboard

[Baarsgaard]

Followed the same pattern as #1686 for the CustomUID.
As discussed in the weekly sync, spec.uid or metadata.uid will always overwrite dashboardModel.uid.

Thanks to the existing Status.UID update detection, existing dashboards are automatically migrated to using metadata.uid by the operator.

Local procedure for testing

Chainsaw tests

make e2e-local-gh-actions TESTS=immutable_uids

Manual

make start-kind
make deploy
kind export kubeconfig --name kind-grafana

# Test resources
kubectl apply -f https://raw.githubusercontent.com/grafana-operator/grafana-operator/master/examples/basic/resources.yaml
kubectl apply -f ../test-dashboard.yaml

# Check status:
# dashboards
#   - default/dashboard-uid/dashUID
kubectl get grafanas -A -o yaml

# Build and apply image to cluster
make ko-build-kind
IMG=ko.local/grafana/grafana-operator make deploy
kubectl patch deploy -n grafana-operator-system grafana-operator-controller-manager-v5  --type='json' -p='[{"op": "replace", "path": "/spec/template/spec/containers/0/imagePullPolicy", "value":"IfNotPresent"}]'

# Check status:
# dashboards
#   - default/dashboard-uid/dashUID -> specUID
kubectl get grafanas -A -o yaml

../test-dashboard.yaml

apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaDashboard
metadata:
  name: dashboard-uid
spec:
  instanceSelector:
    matchLabels:
      dashboards: "grafana"
  uid: specUID
  resyncPeriod: 10s
  json: >
    {
      "id": null,
      "uid": "dashUID",
      "title": "Simple Dashboard",
      "tags": [],
      "style": "dark",
      "timezone": "browser",
      "editable": true,
      "hideControls": false,
      "graphTooltip": 1,
      "panels": [],
      "time": {
        "from": "now-6h",
        "to": "now"
      },
      "timepicker": {
        "time_options": [],
        "refresh_intervals": []
      },
      "templating": {
        "list": []
      },
      "annotations": {
        "list": []
      },
      "refresh": "5s",
      "schemaVersion": 17,
      "version": 0,
      "links": []
    }

• Chainsaw tests for immutability of spec.uid
• Chainsaw tests for expected status on Grafana instance
• Update dashboard UID documentation

[kcolford]

is it possible for this to only override the uid if a uid is not already configured? some collections of dashboards already have a uid in them and then link to each other through that but other dashboards have no uid at all and it would helpful to make sure that the uid can be deterministic if the dashboard's uid isn't present 🤔

[Baarsgaard]

Just to sum up the behaviours:

1. Master branch: spec.json?.uid > metadata.uid
   Allows modifying dashboard UID with fallback to metadata.uid.

2. This PR: spec.uid > metadata.uid
   Resulting UID is immutable, spec.json?.uid is entirely ignored

3. Your suggestion spec.uid > spec.json?.uid > metadata.uid (I think?)
   spec.json?.uid is mutable only when spec.uid is not defined?

All of which has their drawbacks, notably:

1. UID collisions require downloading remotely sourced dashboards getting rid of the benefit of updates.
2. Linking between Dashboards need to have their UIDs hardcoded in the spec.uid to not break the links.
   Requires a delete and create of the resource to change the set UID
3. Same problem as 1 except the workaround is specifying the spec.uid with no other drawbacks to my knowledge, unless the links change?

Personally I don't see much difference between 2 and 3 except they introduce annoyances in two different use cases.
Additionally, there's a slightly higher maintenance burden of 3 due to extra tests and checks necessary.

Yes, it's possible, but I don't know if its the right direction to go in.

[kcolford]

@Baarsgaard the scenario I’m thinking of is how to maintain multiple k8s clusters all with Grafana operator running on them, pointing to what should ostensibly be the “same” Grafana. Having something deterministic is better than using the metadata uid. Although maybe that’s not the best use of this feature and I should instead open a different PR that directly addresses that.

[Baarsgaard]

@kcolford The opposite of my use case where I have 1 Grafana operator with multiple Grafana instances in tens of other clusters!
Would your case be covered by specifying the spec.uid on every GrafanaDashboard manifest as that will overwrite the cr.uid

[kcolford]

@Baarsgaard I think that could work, but it could get messy with determining whether a dashboard already specifies a uid or not. Definitely still possible though, I was just hoping to avoid some extra work.

[theSuess]

Thanks for the PR! From a technical standpoint, this looks great. However, in its current implementation, this would break compatibility with existing setups that rely on the UID being taken from the dashboard spec.

To avoid this, we should only override the UID when the field is actually set.
Once that's implemented, I'm more than happy to merge this!

[Baarsgaard]

@theSuess Sure, but in this case, should the spec.uid field still be immutable?
Or should it be left mutable and thereby different from the other CRs?
additionally, will this be the intended behaviour for the eventual v1 version of the CR?

[theSuess]

It should be left immutable, since editing the UID still shouldn't be allowed.
If possible, it would also be great to forbid setting the field after creation, but I'm not sure if that's possible

[Baarsgaard]

It is possible, part of the Chainsaw tests validate that behaviour right now!
Alright, I will see to fixing it such that dashboard provided uids are sourced before the metadata.uid
This should align with your usecase as well @kcolford

[Baarsgaard]

Alright @theSuess I think that's it?
Please do a review from scratch as I ended up reverting a lot of the uid handling in the dashboards controller due to the previous implementation being more aligned with the cr.CustomUIDOrUID function.

[theSuess]

Code looks good at first glance - will try it out tomorrow & hopefully also merge it/release a new version :)

[theSuess]

I've cleaned up the commit history a bit and queued the PR for merge - thanks again!