Merge pull request #1600 from grafana/feat/folder-conditions

fix: improve nested folder handling

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

api/v1beta1/zz_generated.deepcopy.go

@@ -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
@@ -134,11 +203,8 @@ spec:
                 description: Last time the folder was resynced
                 format: date-time
                 type: string
-              parentFolderUID:
-                description: |-
-                  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

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

@@ -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/controller_shared.go

@@ -20,12 +20,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
-	"time"
 
 	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"
 	ctrl "sigs.k8s.io/controller-runtime"
 
@@ -143,29 +140,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/grafanaalertrulegroup_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
@@ -175,16 +180,35 @@ func (r *GrafanaFolderReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		controllerLog.Error(err, "error getting grafana folder cr")
 		return ctrl.Result{RequeueAfter: RequeueDelay}, err
 	}
+	defer func() {
+		if err := r.UpdateStatus(ctx, folder); err != nil {
+			r.Log.Error(err, "updating status")
+		}
+	}()
+
+	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)
+		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,18 +224,20 @@ 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 {
+	if len(applyErrors) != 0 {
 		return ctrl.Result{RequeueAfter: RequeueDelay}, nil
 	}
 
 	if folder.ResyncPeriodHasElapsed() {
 		folder.Status.LastResync = metav1.Time{Time: time.Now()}
 	}
-	return ctrl.Result{RequeueAfter: folder.GetResyncPeriod()}, r.UpdateStatus(ctx, folder)
+	return ctrl.Result{RequeueAfter: folder.GetResyncPeriod()}, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.
@@ -295,13 +321,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 +354,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 +400,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++
 	}