fix: improve nested folder handling

This fixes some issues around the move logic. The underlying issue was the way
in which folder existance was checked. As the search folder endpoint does _not_
return nested folders, moving between folders was not possible. This fix prefers
matching by UIDs first.

Note that the introduction of nested folders implies that they will only work
with folders created by the operator as names no longer need to be unique and
the only way to keep track is to use the UID.

Author: theSuess

api/v1beta1/grafanafolder_types.go

@@ -65,10 +65,8 @@ type GrafanaFolderStatus struct {
 	// The folder instanceSelector can't find matching grafana instances
 	NoMatchingInstances bool `json:"NoMatchingInstances,omitempty"`
 	// Last time the folder was resynced
-	LastResync metav1.Time `json:"lastResync,omitempty"`
-	// UID of the parent folder where the folder is created.
-	// Will be empty if the folder is deployed at the root level
-	ParentFolderUID string `json:"parentFolderUID,omitempty"`
+	LastResync metav1.Time        `json:"lastResync,omitempty"`
+	Conditions []metav1.Condition `json:"conditions"`
 }
 
 //+kubebuilder:object:root=true
@@ -118,10 +116,6 @@ func (in *GrafanaFolder) Unchanged() bool {
 	return in.Hash() == in.Status.Hash
 }
 
-func (in *GrafanaFolder) Moved() bool {
-	return in.Spec.ParentFolderUID != in.Status.ParentFolderUID
-}
-
 func (in *GrafanaFolder) IsAllowCrossNamespaceImport() bool {
 	if in.Spec.AllowCrossNamespaceImport != nil {
 		return *in.Spec.AllowCrossNamespaceImport

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

@@ -125,6 +125,75 @@ spec:
                 description: The folder instanceSelector can't find matching grafana
                   instances
                 type: boolean
+              conditions:
+                items:
+                  description: "Condition contains details for one aspect of the current
+                    state of this API Resource.\n---\nThis struct is intended for
+                    direct use as an array at the field path .status.conditions.  For
+                    example,\n\n\n\ttype FooStatus struct{\n\t    // Represents the
+                    observations of a foo's current state.\n\t    // Known .status.conditions.type
+                    are: \"Available\", \"Progressing\", and \"Degraded\"\n\t    //
+                    +patchMergeKey=type\n\t    // +patchStrategy=merge\n\t    // +listType=map\n\t
+                    \   // +listMapKey=type\n\t    Conditions []metav1.Condition `json:\"conditions,omitempty\"
+                    patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`\n\n\n\t
+                    \   // other fields\n\t}"
+                  properties:
+                    lastTransitionTime:
+                      description: |-
+                        lastTransitionTime is the last time the condition transitioned from one status to another.
+                        This should be when the underlying condition changed.  If that is not known, then using the time when the API field changed is acceptable.
+                      format: date-time
+                      type: string
+                    message:
+                      description: |-
+                        message is a human readable message indicating details about the transition.
+                        This may be an empty string.
+                      maxLength: 32768
+                      type: string
+                    observedGeneration:
+                      description: |-
+                        observedGeneration represents the .metadata.generation that the condition was set based upon.
+                        For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date
+                        with respect to the current state of the instance.
+                      format: int64
+                      minimum: 0
+                      type: integer
+                    reason:
+                      description: |-
+                        reason contains a programmatic identifier indicating the reason for the condition's last transition.
+                        Producers of specific condition types may define expected values and meanings for this field,
+                        and whether the values are considered a guaranteed API.
+                        The value should be a CamelCase string.
+                        This field may not be empty.
+                      maxLength: 1024
+                      minLength: 1
+                      pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
+                      type: string
+                    status:
+                      description: status of the condition, one of True, False, Unknown.
+                      enum:
+                      - "True"
+                      - "False"
+                      - Unknown
+                      type: string
+                    type:
+                      description: |-
+                        type of condition in CamelCase or in foo.example.com/CamelCase.
+                        ---
+                        Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
+                        useful (see .node.status.conditions), the ability to deconflict is important.
+                        The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
+                      maxLength: 316
+                      pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
+                      type: string
+                  required:
+                  - lastTransitionTime
+                  - message
+                  - reason
+                  - status
+                  - type
+                  type: object
+                type: array
               hash:
                 description: |-
                   INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
@@ -139,6 +208,8 @@ spec:
                   UID of the parent folder where the folder is created.
                   Will be empty if the folder is deployed at the root level
                 type: string
+            required:
+            - conditions
             type: object
         type: object
     served: true

controllers/controller_shared.go

@@ -4,7 +4,9 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"slices"
+	"strings"
 	"time"
 
 	"github.com/grafana/grafana-operator/v5/api/v1beta1"
@@ -22,6 +24,7 @@ const grafanaFinalizer = "operator.grafana.com/finalizer"
 const (
 	conditionNoMatchingInstance = "NoMatchingInstance"
 	conditionNoMatchingFolder   = "NoMatchingFolder"
+	conditionInvalidSpec        = "InvalidSpec"
 )
 
 //+kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;update;patch;delete
@@ -143,6 +146,23 @@ func removeNoMatchingFolder(conditions *[]metav1.Condition) {
 	meta.RemoveStatusCondition(conditions, conditionNoMatchingFolder)
 }
 
+func setInvalidSpec(conditions *[]metav1.Condition, generation int64, reason, message string) {
+	meta.SetStatusCondition(conditions, metav1.Condition{
+		Type:               conditionInvalidSpec,
+		Status:             "True",
+		ObservedGeneration: generation,
+		LastTransitionTime: metav1.Time{
+			Time: time.Now(),
+		},
+		Reason:  reason,
+		Message: message,
+	})
+}
+
+func removeInvalidSpec(conditions *[]metav1.Condition) {
+	meta.RemoveStatusCondition(conditions, conditionInvalidSpec)
+}
+
 func ignoreStatusUpdates() predicate.Predicate {
 	return predicate.Funcs{
 		UpdateFunc: func(e event.UpdateEvent) bool {
@@ -151,3 +171,30 @@ func ignoreStatusUpdates() predicate.Predicate {
 		},
 	}
 }
+
+func buildSynchronizedCondition(resource string, syncType string, generation int64, applyErrors map[string]string, total int) metav1.Condition {
+	condition := metav1.Condition{
+		Type:               syncType,
+		ObservedGeneration: generation,
+		LastTransitionTime: metav1.Time{
+			Time: time.Now(),
+		},
+	}
+
+	if len(applyErrors) == 0 {
+		condition.Status = "True"
+		condition.Reason = "ApplySuccessful"
+		condition.Message = fmt.Sprintf("%s was successfully applied to %d instances", resource, total)
+	} else {
+		condition.Status = "False"
+		condition.Reason = "ApplyFailed"
+
+		var sb strings.Builder
+		for i, err := range applyErrors {
+			sb.WriteString(fmt.Sprintf("\n- %s: %s", i, err))
+		}
+
+		condition.Message = fmt.Sprintf("%s failed to be applied for %d out of %d instances. Errors:%s", resource, len(applyErrors), total, sb.String())
+	}
+	return condition
+}

controllers/grafanaalertrulegroup_controller.go

@@ -143,29 +143,7 @@ func (r *GrafanaAlertRuleGroupReconciler) Reconcile(ctx context.Context, req ctr
 			applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error()
 		}
 	}
-	condition := metav1.Condition{
-		Type:               conditionAlertGroupSynchronized,
-		ObservedGeneration: group.Generation,
-		LastTransitionTime: metav1.Time{
-			Time: time.Now(),
-		},
-	}
-
-	if len(applyErrors) == 0 {
-		condition.Status = "True"
-		condition.Reason = "ApplySuccessful"
-		condition.Message = fmt.Sprintf("Alert Rule Group was successfully applied to %d instances", len(instances))
-	} else {
-		condition.Status = "False"
-		condition.Reason = "ApplyFailed"
-
-		var sb strings.Builder
-		for i, err := range applyErrors {
-			sb.WriteString(fmt.Sprintf("\n- %s: %s", i, err))
-		}
-
-		condition.Message = fmt.Sprintf("Alert Rule Group failed to be applied for %d out of %d instances. Errors:%s", len(applyErrors), len(instances), sb.String())
-	}
+	condition := buildSynchronizedCondition("Alert Rule Group", conditionAlertGroupSynchronized, group.Generation, applyErrors, len(instances))
 	meta.SetStatusCondition(&group.Status.Conditions, condition)
 
 	return ctrl.Result{RequeueAfter: group.Spec.ResyncPeriod.Duration}, nil

controllers/grafanafolder_controller.go

@@ -31,6 +31,7 @@ import (
 	client2 "github.com/grafana/grafana-operator/v5/controllers/client"
 	"github.com/grafana/grafana-operator/v5/controllers/metrics"
 	kuberr "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/apimachinery/pkg/runtime"
@@ -41,6 +42,10 @@ import (
 	grafanav1beta1 "github.com/grafana/grafana-operator/v5/api/v1beta1"
 )
 
+const (
+	conditionFolderSynchronized = "FolderSynchronized"
+)
+
 // GrafanaFolderReconciler reconciles a GrafanaFolder object
 type GrafanaFolderReconciler struct {
 	client.Client
@@ -176,15 +181,30 @@ func (r *GrafanaFolderReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{RequeueAfter: RequeueDelay}, err
 	}
 
+	if folder.Spec.ParentFolderUID == string(folder.UID) {
+		setInvalidSpec(&folder.Status.Conditions, folder.Generation, "CyclicParent", "The value of parentFolderUID must not be the uid of the current folder")
+		meta.RemoveStatusCondition(&folder.Status.Conditions, conditionFolderSynchronized)
+		r.UpdateStatus(ctx, folder)
+		return ctrl.Result{}, fmt.Errorf("cyclic folder reference")
+	}
+	removeInvalidSpec(&folder.Status.Conditions)
+
 	instances, err := r.GetMatchingFolderInstances(ctx, folder, r.Client)
 	if err != nil {
-		controllerLog.Error(err, "could not find matching instances", "name", folder.Name, "namespace", folder.Namespace)
-		return ctrl.Result{RequeueAfter: RequeueDelay}, err
+		setNoMatchingInstance(&folder.Status.Conditions, folder.Generation, "ErrFetchingInstances", fmt.Sprintf("error occurred during fetching of instances: %s", err.Error()))
+		meta.RemoveStatusCondition(&folder.Status.Conditions, conditionFolderSynchronized)
+		r.Log.Error(err, "could not find matching instances")
+		return ctrl.Result{}, err
 	}
-
+	if len(instances.Items) == 0 {
+		setNoMatchingInstance(&folder.Status.Conditions, folder.Generation, "EmptyAPIReply", "Instances could not be fetched, reconciliation will be retried")
+		meta.RemoveStatusCondition(&folder.Status.Conditions, conditionFolderSynchronized)
+		return ctrl.Result{}, fmt.Errorf("no instances found")
+	}
+	removeNoMatchingInstance(&folder.Status.Conditions)
 	controllerLog.Info("found matching Grafana instances for folder", "count", len(instances.Items))
 
-	success := true
+	applyErrors := make(map[string]string)
 	for _, grafana := range instances.Items {
 		// check if this is a cross namespace import
 		if grafana.Namespace != folder.Namespace && !folder.IsAllowCrossNamespaceImport() {
@@ -200,12 +220,14 @@ func (r *GrafanaFolderReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		err = r.onFolderCreated(ctx, &grafana, folder)
 		if err != nil {
 			controllerLog.Error(err, "error reconciling folder", "folder", folder.Name, "grafana", grafana.Name)
-			success = false
+			applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error()
 		}
 	}
+	condition := buildSynchronizedCondition("Folder", conditionFolderSynchronized, folder.Generation, applyErrors, len(instances.Items))
+	meta.SetStatusCondition(&folder.Status.Conditions, condition)
 
-	if !success {
-		return ctrl.Result{RequeueAfter: RequeueDelay}, nil
+	if len(applyErrors) != 0 {
+		return ctrl.Result{RequeueAfter: RequeueDelay}, r.UpdateStatus(ctx, folder)
 	}
 
 	if folder.ResyncPeriodHasElapsed() {
@@ -295,13 +317,13 @@ func (r *GrafanaFolderReconciler) onFolderCreated(ctx context.Context, grafana *
 		return err
 	}
 
-	exists, remoteUID, err := r.Exists(grafanaClient, cr)
+	exists, remoteUID, remoteParent, err := r.Exists(grafanaClient, cr)
 	if err != nil {
 		return err
 	}
 
 	// always update after resync period has elapsed even if cr is unchanged.
-	if exists && cr.Unchanged() && !cr.ResyncPeriodHasElapsed() && !cr.Moved() {
+	if exists && cr.Unchanged() && !cr.ResyncPeriodHasElapsed() && cr.Spec.ParentFolderUID == remoteParent {
 		return nil
 	}
 
@@ -328,7 +350,7 @@ func (r *GrafanaFolderReconciler) onFolderCreated(ctx context.Context, grafana *
 			}
 		}
 
-		if cr.Moved() {
+		if cr.Spec.ParentFolderUID != remoteParent {
 			_, err = grafanaClient.Folders.MoveFolder(remoteUID, &models.MoveFolderCommand{ //nolint
 				ParentUID: cr.Spec.ParentFolderUID,
 			})
@@ -374,30 +396,35 @@ func (r *GrafanaFolderReconciler) onFolderCreated(ctx context.Context, grafana *
 
 func (r *GrafanaFolderReconciler) UpdateStatus(ctx context.Context, cr *grafanav1beta1.GrafanaFolder) error {
 	cr.Status.Hash = cr.Hash()
-	cr.Status.ParentFolderUID = cr.Spec.ParentFolderUID
 	return r.Client.Status().Update(ctx, cr)
 }
 
-func (r *GrafanaFolderReconciler) Exists(client *genapi.GrafanaHTTPAPI, cr *grafanav1beta1.GrafanaFolder) (bool, string, error) {
+// Check if the folder exists. Matches UID first and fall back to title. Title matching only works for non-nested folders
+func (r *GrafanaFolderReconciler) Exists(client *genapi.GrafanaHTTPAPI, cr *grafanav1beta1.GrafanaFolder) (bool, string, string, error) {
 	title := cr.GetTitle()
 	uid := string(cr.UID)
 
+	uidResp, err := client.Folders.GetFolderByUID(uid)
+	if err == nil {
+		return true, uidResp.Payload.UID, uidResp.Payload.ParentUID, nil
+	}
+
 	page := int64(1)
 	limit := int64(10000)
 	for {
-		params := folders.NewGetFoldersParams().WithPage(&page).WithLimit(&limit).WithParentUID(&cr.Status.ParentFolderUID)
+		params := folders.NewGetFoldersParams().WithPage(&page).WithLimit(&limit)
 
 		foldersResp, err := client.Folders.GetFolders(params)
 		if err != nil {
-			return false, "", err
+			return false, "", "", err
 		}
 		for _, folder := range foldersResp.Payload {
-			if folder.UID == uid || strings.EqualFold(folder.Title, title) {
-				return true, folder.UID, nil
+			if strings.EqualFold(folder.Title, title) {
+				return true, folder.UID, folder.ParentUID, nil
 			}
 		}
 		if len(foldersResp.Payload) < int(limit) {
-			return false, "", nil
+			return false, "", "", nil
 		}
 		page++
 	}