Merge pull request #1694 from Baarsgaard/dashboard_uid_refactor

Implement `spec.uid` for `GrafanaDashboard`

Makefile

@@ -21,7 +21,7 @@ REGISTRY ?= ghcr.io
 ORG ?= grafana
 IMG ?= $(REGISTRY)/$(ORG)/grafana-operator:v$(VERSION)
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.25.0
+ENVTEST_K8S_VERSION = 1.28.0
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))
@@ -30,7 +30,7 @@ else
 GOBIN=$(shell go env GOBIN)
 endif
 
-CHAINSAW_VERSION ?= v0.1.9
+CHAINSAW_VERSION ?= v0.2.10
 
 # Checks if chainsaw is in your PATH
 ifneq ($(shell which chainsaw),)
@@ -45,7 +45,7 @@ SHELL = /usr/bin/env bash -o pipefail
 .SHELLFLAGS = -ec
 
 .PHONY: all
-all: manifests test api-docs
+all: manifests test api-docs helm/docs
 
 ##@ General
 
@@ -261,9 +261,18 @@ bundle/redhat: BUNDLE_GEN_FLAGS += --use-image-digests
 bundle/redhat: bundle
 
 # e2e
+.PHONY: e2e-kind
+e2e-kind:
+ifeq (,$(shell kind get clusters $(KIND_CLUSTER_NAME)))
+	$(KIND) --kubeconfig="${KUBECONFIG}" create cluster --image=kindest/node:v$(ENVTEST_K8S_VERSION) --config tests/e2e/kind.yaml
+endif
+
+.PHONY: e2e-local-gh-actions
+e2e-local-gh-actions: e2e-kind ko-build-kind e2e
+
 .PHONY: e2e
 e2e: chainsaw install deploy-chainsaw ## Run e2e tests using chainsaw.
-	$(CHAINSAW) test --test-dir ./tests/e2e
+	$(CHAINSAW) test --test-dir ./tests/e2e/$(TESTS)
 
 # Find or download chainsaw
 chainsaw:

api/v1beta1/grafanadashboard_types.go

@@ -60,7 +60,13 @@ type GrafanaDashboardUrlAuthorization struct {
 // GrafanaDashboardSpec defines the desired state of GrafanaDashboard
 // +kubebuilder:validation:XValidation:rule="(has(self.folderUID) && !(has(self.folderRef))) || (has(self.folderRef) && !(has(self.folderUID))) || !(has(self.folderRef) && (has(self.folderUID)))", message="Only one of folderUID or folderRef can be declared at the same time"
 // +kubebuilder:validation:XValidation:rule="(has(self.folder) && !(has(self.folderRef) || has(self.folderUID))) || !(has(self.folder))", message="folder field cannot be set when folderUID or folderRef is already declared"
+// +kubebuilder:validation:XValidation:rule="((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) && has(self.uid)))", message="spec.uid is immutable"
 type GrafanaDashboardSpec struct {
+	// Manually specify the uid for the dashboard, overwrites uids already present in the json model
+	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.uid is immutable"
+	CustomUID string `json:"uid,omitempty"`
+
 	// dashboard json
 	// +optional
 	Json string `json:"json,omitempty"`
@@ -143,7 +149,7 @@ type GrafanaDashboardSpec struct {
 
 type GrafanaDashboardEnv struct {
 	Name string `json:"name"`
-	// Inline evn value
+	// Inline env value
 	// +optional
 	Value string `json:"value,omitempty"`
 	// Reference on value source, might be the reference on a secret or config map
@@ -222,6 +228,19 @@ func (in *GrafanaDashboard) FolderUID() string {
 	return in.Spec.FolderUID
 }
 
+// Wrapper around CustomUID, dashboardModelUID or default metadata.uid
+func (in *GrafanaDashboard) CustomUIDOrUID(dashboardUID string) string {
+	if in.Spec.CustomUID != "" {
+		return in.Spec.CustomUID
+	}
+
+	if dashboardUID != "" {
+		return dashboardUID
+	}
+
+	return string(in.ObjectMeta.UID)
+}
+
 // FolderNamespace implements FolderReferencer.
 func (in *GrafanaDashboard) FolderNamespace() string {
 	return in.Namespace
@@ -331,9 +350,7 @@ func (in *GrafanaDashboard) IsUpdatedUID(uid string) bool {
 		return false
 	}
 
-	if uid == "" {
-		uid = string(in.UID)
-	}
+	uid = in.CustomUIDOrUID(uid)
 
 	return in.Status.UID != uid
 }

api/v1beta1/grafanadashboard_types_test.go

@@ -1,6 +1,7 @@
 package v1beta1
 
 import (
+	"encoding/json"
 	"testing"
 	"time"
 
@@ -105,61 +106,152 @@ func Test_Gzip(t *testing.T) {
 
 func TestGrafanaDashboardIsUpdatedUID(t *testing.T) {
 	crUID := "crUID"
+	dashUID := "dashUID"
+	specUID := "specUID"
 	tests := []struct {
 		name         string
 		crUID        string
 		statusUID    string
 		dashboardUID string
+		specUID      string
 		want         bool
 	}{
+		// Validate always false when statusUID is empty
+		// Since dashboardUID is ignoredk the only variable is customUID
 		{
-			name:         "Status.UID and dashboard UID are empty",
+			name:         "Empty StatusUID always results in false",
 			crUID:        crUID,
 			statusUID:    "",
-			dashboardUID: "newUID",
+			dashboardUID: "",
+			specUID:      "",
+			want:         false,
+		},
+		{
+			name:         "Always false when statusUID is empty regardless of dashUID being set",
+			crUID:        crUID,
+			statusUID:    "",
+			dashboardUID: dashUID,
+			specUID:      "",
+			want:         false,
+		},
+		{
+			name:         "Always false when statusUID is empty regardless of customUID being set",
+			crUID:        crUID,
+			statusUID:    "",
+			dashboardUID: "",
+			specUID:      specUID,
 			want:         false,
 		},
 		{
-			name:         "Status.UID is empty, dashboard UID is not",
+			name:         "Always false when statusUID is empty regardless of customUID or dashUID being set",
 			crUID:        crUID,
 			statusUID:    "",
-			dashboardUID: "newUID",
+			dashboardUID: dashUID,
+			specUID:      specUID,
 			want:         false,
 		},
+		// Validate that crUID is always overwritten by dashUID or customUID
+		// dashboardUID is always overwritten by customUID which falls back to crUID
 		{
-			name:         "Status.UID is not empty (same as CR uid), new UID is empty",
+			name:         "DashboardUID and customUID empty",
 			crUID:        crUID,
 			statusUID:    crUID,
 			dashboardUID: "",
+			specUID:      "",
+			want:         false,
+		},
+		{
+			name:         "DashboardUID set and customUID empty",
+			crUID:        crUID,
+			statusUID:    dashUID,
+			dashboardUID: dashUID,
+			specUID:      "",
+			want:         false,
+		},
+		{
+			name:         "DashboardUID set and customUID set",
+			crUID:        crUID,
+			statusUID:    specUID,
+			dashboardUID: dashUID,
+			specUID:      specUID,
+			want:         false,
+		},
+		{
+			name:         "DashboardUID empty and customUID set",
+			crUID:        crUID,
+			statusUID:    specUID,
+			dashboardUID: "",
+			specUID:      specUID,
+			want:         false,
+		},
+		// Validate updates are detected correctly
+		{
+			name:         "DashboardUID updated and customUID empty",
+			crUID:        crUID,
+			statusUID:    crUID,
+			dashboardUID: dashUID,
+			specUID:      "",
+			want:         true,
+		},
+		{
+			name:         "DashboardUID updated and customUID set",
+			crUID:        crUID,
+			statusUID:    specUID,
+			dashboardUID: dashUID,
+			specUID:      specUID,
 			want:         false,
 		},
 		{
-			name:         "Status.UID is not empty (different from CR uid), new UID is empty",
+			name:         "new dashUID and no customUID",
+			crUID:        crUID,
+			statusUID:    "oldUID",
+			dashboardUID: dashUID,
+			specUID:      "",
+			want:         true,
+		},
+		{
+			name:         "dashUID removed and no customUID",
 			crUID:        crUID,
 			statusUID:    "oldUID",
 			dashboardUID: "",
+			specUID:      "",
 			want:         true,
 		},
+		// Validate that statusUID detection works even in impossible cases expecting cr or customUID to change
 		{
-			name:         "Status.UID is not empty, new UID is different",
+			name:         "IMPOSSIBLE: Old status with new customUID",
 			crUID:        crUID,
 			statusUID:    "oldUID",
-			dashboardUID: "newUID",
+			dashboardUID: "",
+			specUID:      specUID,
+			want:         true,
+		},
+		{
+			name:         "IMPOSSIBLE: Old Status with all UIDs being equal",
+			crUID:        crUID,
+			statusUID:    "oldUID",
+			dashboardUID: crUID,
+			specUID:      crUID,
 			want:         true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			cr := getDashboardCR(t, tt.crUID, tt.statusUID)
-			got := cr.IsUpdatedUID(tt.dashboardUID)
+			cr := getDashboardCR(t, tt.crUID, tt.statusUID, tt.specUID, tt.dashboardUID)
+			uid := cr.CustomUIDOrUID(tt.dashboardUID)
+
+			got := cr.IsUpdatedUID(uid)
 			assert.Equal(t, tt.want, got)
 		})
 	}
 }
 
-func getDashboardCR(t *testing.T, crUID string, statusUID string) GrafanaDashboard {
+func getDashboardCR(t *testing.T, crUID string, statusUID string, specUID string, dashUID string) GrafanaDashboard {
 	t.Helper()
+	var dashboardModel map[string]interface{} = make(map[string]interface{})
+	dashboardModel["uid"] = dashUID
+	dashboard, _ := json.Marshal(dashboardModel) //nolint:errcheck
 
 	cr := GrafanaDashboard{
 		TypeMeta: metav1.TypeMeta{},
@@ -174,6 +266,8 @@ func getDashboardCR(t *testing.T, crUID string, statusUID string) GrafanaDashboa
 					"dashboard": "grafana",
 				},
 			},
+			CustomUID: specUID,
+			Json:      string(dashboard),
 		},
 		Status: GrafanaDashboardStatus{
 			UID: statusUID,

config/crd/bases/grafana.integreatly.org_grafanadashboards.yaml

@@ -155,7 +155,7 @@ spec:
                     name:
                       type: string
                     value:
-                      description: Inline evn value
+                      description: Inline env value
                       type: string
                     valueFrom:
                       description: Reference on value source, might be the reference
@@ -328,6 +328,13 @@ spec:
                 format: duration
                 pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$
                 type: string
+              uid:
+                description: Manually specify the uid for the dashboard, overwrites
+                  uids already present in the json model
+                type: string
+                x-kubernetes-validations:
+                - message: spec.uid is immutable
+                  rule: self == oldSelf
               url:
                 description: dashboard url
                 type: string
@@ -398,6 +405,9 @@ spec:
                 declared
               rule: (has(self.folder) && !(has(self.folderRef) || has(self.folderUID)))
                 || !(has(self.folder))
+            - message: spec.uid is immutable
+              rule: ((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) &&
+                has(self.uid)))
           status:
             description: GrafanaDashboardStatus defines the observed state of GrafanaDashboard
             properties:

controllers/dashboard_controller.go

@@ -207,6 +207,7 @@ func (r *GrafanaDashboardReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		return ctrl.Result{RequeueAfter: RequeueDelay}, nil
 	}
 
+	// Retrieving the model before the loop ensures to exit early in case of failure and not fail once per matching instance
 	dashboardModel, hash, err := r.getDashboardModel(cr, dashboardJson)
 	if err != nil {
 		controllerLog.Error(err, "failed to prepare dashboard model", "dashboard", cr.Name)
@@ -579,11 +580,7 @@ func (r *GrafanaDashboardReconciler) getDashboardModel(cr *v1beta1.GrafanaDashbo
 	dashboardModel["id"] = nil
 
 	uid, _ := dashboardModel["uid"].(string) //nolint:errcheck
-	if uid == "" {
-		uid = string(cr.UID)
-	}
-
-	dashboardModel["uid"] = uid
+	dashboardModel["uid"] = cr.CustomUIDOrUID(uid)
 
 	return dashboardModel, fmt.Sprintf("%x", hash.Sum(nil)), nil
 }

deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadashboards.yaml

@@ -155,7 +155,7 @@ spec:
                     name:
                       type: string
                     value:
-                      description: Inline evn value
+                      description: Inline env value
                       type: string
                     valueFrom:
                       description: Reference on value source, might be the reference
@@ -328,6 +328,13 @@ spec:
                 format: duration
                 pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$
                 type: string
+              uid:
+                description: Manually specify the uid for the dashboard, overwrites
+                  uids already present in the json model
+                type: string
+                x-kubernetes-validations:
+                - message: spec.uid is immutable
+                  rule: self == oldSelf
               url:
                 description: dashboard url
                 type: string
@@ -398,6 +405,9 @@ spec:
                 declared
               rule: (has(self.folder) && !(has(self.folderRef) || has(self.folderUID)))
                 || !(has(self.folder))
+            - message: spec.uid is immutable
+              rule: ((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) &&
+                has(self.uid)))
           status:
             description: GrafanaDashboardStatus defines the observed state of GrafanaDashboard
             properties: