diff --git a/api/v1beta1/common.go b/api/v1beta1/common.go index 3872b6c19..84925e2fe 100644 --- a/api/v1beta1/common.go +++ b/api/v1beta1/common.go @@ -39,3 +39,12 @@ type GrafanaCommonSpec struct { // +optional AllowCrossNamespaceImport *bool `json:"allowCrossNamespaceImport,omitempty"` } + +// Common Functions that all CRs should implement, excluding Grafana +// +kubebuilder:object:generate=false +type CommonResource interface { + MatchLabels() *metav1.LabelSelector + MatchNamespace() string + AllowCrossNamespace() bool + ResyncPeriodHasElapsed() bool +} diff --git a/api/v1beta1/grafanafolder_types.go b/api/v1beta1/grafanafolder_types.go index cbde5117f..b2d79b406 100644 --- a/api/v1beta1/grafanafolder_types.go +++ b/api/v1beta1/grafanafolder_types.go @@ -151,13 +151,6 @@ func (in *GrafanaFolder) Unchanged() bool { return in.Hash() == in.Status.Hash } -func (in *GrafanaFolder) IsAllowCrossNamespaceImport() bool { - if in.Spec.AllowCrossNamespaceImport != nil { - return *in.Spec.AllowCrossNamespaceImport - } - return false -} - func (in *GrafanaFolder) GetTitle() string { if in.Spec.Title != "" { return in.Spec.Title @@ -170,3 +163,18 @@ func (in *GrafanaFolder) ResyncPeriodHasElapsed() bool { deadline := in.Status.LastResync.Add(in.Spec.ResyncPeriod.Duration) return time.Now().After(deadline) } + +func (in *GrafanaFolder) MatchLabels() *metav1.LabelSelector { + return in.Spec.InstanceSelector +} + +func (in *GrafanaFolder) MatchNamespace() string { + return in.ObjectMeta.Namespace +} + +func (in *GrafanaFolder) AllowCrossNamespace() bool { + if in.Spec.AllowCrossNamespaceImport != nil { + return *in.Spec.AllowCrossNamespaceImport + } + return false +} diff --git a/controllers/controller_shared.go b/controllers/controller_shared.go index e3a4cc7d4..8c112737d 100644 --- a/controllers/controller_shared.go +++ b/controllers/controller_shared.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/go-logr/logr" operatorapi "github.com/grafana/grafana-operator/v5/api" "github.com/grafana/grafana-operator/v5/api/v1beta1" grafanav1beta1 "github.com/grafana/grafana-operator/v5/api/v1beta1" @@ -31,11 +32,11 @@ const ( conditionInvalidSpec = "InvalidSpec" ) -const annotationAppliedNotificationPolicy = "operator.grafana.com/applied-notificationpolicy" - //+kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;update;patch;delete +// Gets all instances matching labelSelector func GetMatchingInstances(ctx context.Context, k8sClient client.Client, labelSelector *metav1.LabelSelector) (v1beta1.GrafanaList, error) { + // Should never happen, sanity check if labelSelector == nil { return v1beta1.GrafanaList{}, nil } @@ -58,6 +59,91 @@ func GetMatchingInstances(ctx context.Context, k8sClient client.Client, labelSel return selectedList, err } +// Only matching instances in the scope of the resource are returned +// Resources with allowCrossNamespaceImport expands the scope to the entire cluster +// Intended to be used in reconciler functions +func GetScopedMatchingInstances(log logr.Logger, ctx context.Context, k8sClient client.Client, cr v1beta1.CommonResource) ([]v1beta1.Grafana, error) { + instanceSelector := cr.MatchLabels() + + // Should never happen, sanity check + if instanceSelector == nil { + return []v1beta1.Grafana{}, nil + } + + opts := []client.ListOption{ + // Matches all instances when MatchLabels is undefined + client.MatchingLabels(instanceSelector.MatchLabels), + } + + if !cr.AllowCrossNamespace() { + // Only query resource namespace + opts = append(opts, client.InNamespace(cr.MatchNamespace())) + } + + var list v1beta1.GrafanaList + err := k8sClient.List(ctx, &list, opts...) + if err != nil || len(list.Items) == 0 { + return []v1beta1.Grafana{}, err + } + + selectedList := []v1beta1.Grafana{} + var unready_instances []string + for _, instance := range list.Items { + // Matches all instances when MatchExpressions is undefined + selected := labelsSatisfyMatchExpressions(instance.Labels, instanceSelector.MatchExpressions) + if !selected { + continue + } + // admin url is required to interact with Grafana + // the instance or route might not yet be ready + if instance.Status.Stage != v1beta1.OperatorStageComplete || instance.Status.StageStatus != v1beta1.OperatorStageResultSuccess { + unready_instances = append(unready_instances, instance.Name) + continue + } + selectedList = append(selectedList, instance) + } + if len(unready_instances) > 1 { + log.Info("Grafana instances not ready", "instances", unready_instances) + } + + return selectedList, nil +} + +// Same as GetScopedMatchingInstances, except the scope is always global +// Intended to be used in finalizer and onDelete functions due to allowCrossNamespaceImport being a mutable field +// Not using this may leave behind resources in instances no longer in scope. +func GetAllMatchingInstances(ctx context.Context, k8sClient client.Client, cr v1beta1.CommonResource) ([]v1beta1.Grafana, error) { + instanceSelector := cr.MatchLabels() + + // Should never happen, sanity check + if instanceSelector == nil { + return []v1beta1.Grafana{}, nil + } + + var list v1beta1.GrafanaList + err := k8sClient.List(ctx, &list, client.MatchingLabels(instanceSelector.MatchLabels)) + if err != nil || len(list.Items) == 0 { + return []v1beta1.Grafana{}, err + } + + selectedList := []v1beta1.Grafana{} + for _, instance := range list.Items { + // Matches all instances when MatchExpressions is undefined + selected := labelsSatisfyMatchExpressions(instance.Labels, instanceSelector.MatchExpressions) + if !selected { + continue + } + // admin url is required to interact with Grafana + // the instance or route might not yet be ready + if instance.Status.Stage != v1beta1.OperatorStageComplete || instance.Status.StageStatus != v1beta1.OperatorStageResultSuccess { + continue + } + selectedList = append(selectedList, instance) + } + + return selectedList, nil +} + // getFolderUID fetches the folderUID from an existing GrafanaFolder CR declared in the specified namespace func getFolderUID(ctx context.Context, k8sClient client.Client, ref operatorapi.FolderReferencer) (string, error) { if ref.FolderUID() != "" { @@ -145,6 +231,28 @@ func ReconcilePlugins(ctx context.Context, k8sClient client.Client, scheme *runt return nil } +// Correctly determine cause of no matching instance from error +func setNoMatchingInstancesCondition(conditions *[]metav1.Condition, generation int64, err error) { + var reason, message string + if err != nil { + reason = "ErrFetchingInstances" + message = fmt.Sprintf("error occurred during fetching of instances: %s", err.Error()) + } else { + reason = "EmptyAPIReply" + message = "Instances could not be fetched, reconciliation will be retried" + } + meta.SetStatusCondition(conditions, metav1.Condition{ + Type: conditionNoMatchingInstance, + Status: "True", + ObservedGeneration: generation, + Reason: reason, + Message: message, + LastTransitionTime: metav1.Time{ + Time: time.Now(), + }, + }) +} + func setNoMatchingInstance(conditions *[]metav1.Condition, generation int64, reason, message string) { meta.SetStatusCondition(conditions, metav1.Condition{ Type: conditionNoMatchingInstance, diff --git a/controllers/controller_shared_test.go b/controllers/controller_shared_test.go index 607972671..c5c71238a 100644 --- a/controllers/controller_shared_test.go +++ b/controllers/controller_shared_test.go @@ -17,10 +17,17 @@ limitations under the License. package controllers import ( + "context" "testing" + "github.com/grafana/grafana-operator/v5/api/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/log" ) func TestLabelsSatisfyMatchExpressions(t *testing.T) { @@ -226,3 +233,122 @@ func TestLabelsSatisfyMatchExpressions(t *testing.T) { }) } } + +var _ = Describe("GetMatchingInstances functions", Ordered, func() { + refTrue := true + refFalse := false + namespace := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "get-matching-test", + }, + } + allowFolder := v1beta1.GrafanaFolder{ + TypeMeta: v1.TypeMeta{ + APIVersion: "grafana.integreatly.org/v1beta1", + Kind: "GrafanaFolder", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "allow-cross-namespace", + Namespace: namespace.Name, + }, + Spec: v1beta1.GrafanaFolderSpec{ + GrafanaCommonSpec: v1beta1.GrafanaCommonSpec{ + AllowCrossNamespaceImport: &refTrue, + InstanceSelector: &v1.LabelSelector{ + MatchLabels: map[string]string{ + "test": "folder", + }, + }, + }, + }, + } + // Create duplicate resources, changing key fields + denyFolder := allowFolder.DeepCopy() + denyFolder.Name = "deny-cross-namespace" + denyFolder.Spec.AllowCrossNamespaceImport = &refFalse + + matchAllFolder := allowFolder.DeepCopy() + matchAllFolder.Name = "invalid-match-labels" + matchAllFolder.Spec.InstanceSelector = &metav1.LabelSelector{} // InstanceSelector is never nil + + DefaultGrafana := v1beta1.Grafana{ + TypeMeta: v1.TypeMeta{ + APIVersion: "grafana.integreatly.org/v1beta1", + Kind: "Grafana", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "instance", + Namespace: "default", + Labels: map[string]string{ + "test": "folder", + }, + }, + Spec: v1beta1.GrafanaSpec{}, + } + matchesNothingGrafana := DefaultGrafana.DeepCopy() + matchesNothingGrafana.Name = "match-nothing-instance" + matchesNothingGrafana.Labels = nil + + secondNamespaceGrafana := DefaultGrafana.DeepCopy() + secondNamespaceGrafana.Name = "second-namespace-instance" + secondNamespaceGrafana.Namespace = namespace.Name + + // Status update is skipped for this + unreadyGrafana := DefaultGrafana.DeepCopy() + unreadyGrafana.Name = "unready-instance" + + ctx := context.Background() + // Pre-create all resources + BeforeAll(func() { // Necessary to use assertions + Expect(k8sClient.Create(ctx, &namespace)).NotTo(HaveOccurred()) + Expect(k8sClient.Create(ctx, &allowFolder)).NotTo(HaveOccurred()) + Expect(k8sClient.Create(ctx, denyFolder)).NotTo(HaveOccurred()) + Expect(k8sClient.Create(ctx, matchAllFolder)).NotTo(HaveOccurred()) + Expect(k8sClient.Create(ctx, unreadyGrafana)).NotTo(HaveOccurred()) + + grafanas := []v1beta1.Grafana{DefaultGrafana, *matchesNothingGrafana, *secondNamespaceGrafana} + for _, instance := range grafanas { + Expect(k8sClient.Create(ctx, &instance)).NotTo(HaveOccurred()) + + // Apply status to pass instance ready check + instance.Status.Stage = v1beta1.OperatorStageComplete + instance.Status.StageStatus = v1beta1.OperatorStageResultSuccess + Expect(k8sClient.Status().Update(ctx, &instance)).ToNot(HaveOccurred()) + } + }) + + Context("Ensure AllowCrossNamespaceImport is upheld by GetScopedMatchingInstances", func() { + testLog := log.FromContext(ctx).WithSink(log.NullLogSink{}) + It("Finds all ready instances when instanceSelector is empty", func() { + instances, err := GetScopedMatchingInstances(testLog, ctx, k8sClient, matchAllFolder) + Expect(err).ToNot(HaveOccurred()) + Expect(instances).To(HaveLen(3)) + }) + It("Finds all ready and Matching instances", func() { + instances, err := GetScopedMatchingInstances(testLog, ctx, k8sClient, &allowFolder) + Expect(err).ToNot(HaveOccurred()) + Expect(instances).ToNot(BeEmpty()) + Expect(instances).To(HaveLen(2)) + }) + It("Finds matching and ready and matching instance in namespace", func() { + instances, err := GetScopedMatchingInstances(testLog, ctx, k8sClient, denyFolder) + Expect(err).ToNot(HaveOccurred()) + Expect(instances).ToNot(BeEmpty()) + Expect(instances).To(HaveLen(1)) + }) + }) + + Context("Ensure AllowCrossNamespaceImport is ignored by GetAllMatchingInstances", func() { + It("Finds all ready instances when instanceSelector is empty", func() { + instances, err := GetAllMatchingInstances(ctx, k8sClient, matchAllFolder) + Expect(err).ToNot(HaveOccurred()) + Expect(instances).To(HaveLen(3)) + }) + It("Finds matching and ready instances ignoring AllowCrossNamespaceImport", func() { + instances, err := GetAllMatchingInstances(ctx, k8sClient, denyFolder) + Expect(err).ToNot(HaveOccurred()) + Expect(instances).ToNot(BeEmpty()) + Expect(instances).To(HaveLen(2)) + }) + }) +}) diff --git a/controllers/grafanafolder_controller.go b/controllers/grafanafolder_controller.go index 0f3ddb034..e7ae74f04 100644 --- a/controllers/grafanafolder_controller.go +++ b/controllers/grafanafolder_controller.go @@ -194,45 +194,32 @@ func (r *GrafanaFolderReconciler) Reconcile(ctx context.Context, req ctrl.Reques } removeInvalidSpec(&folder.Status.Conditions) - instances, err := r.GetMatchingFolderInstances(ctx, folder, r.Client) - if err != nil { - 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") + instances, err := GetScopedMatchingInstances(controllerLog, ctx, r.Client, folder) + if err != nil || len(instances) == 0 { + setNoMatchingInstancesCondition(&folder.Status.Conditions, folder.Generation, err) + folder.Status.NoMatchingInstances = true + controllerLog.Error(err, "could not find matching instances", "name", folder.Name, "namespace", folder.Namespace) + return ctrl.Result{RequeueAfter: RequeueDelay}, nil } + removeNoMatchingInstance(&folder.Status.Conditions) - controllerLog.Info("found matching Grafana instances for folder", "count", len(instances.Items)) + folder.Status.NoMatchingInstances = false + controllerLog.Info("found matching Grafana instances for folder", "count", len(instances)) 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() { - continue - } - + for _, grafana := range instances { grafana := grafana - if grafana.Status.Stage != grafanav1beta1.OperatorStageComplete || grafana.Status.StageStatus != grafanav1beta1.OperatorStageResultSuccess { - controllerLog.Info("grafana instance not ready", "grafana", grafana.Name) - continue - } err = r.onFolderCreated(ctx, &grafana, folder) if err != nil { - controllerLog.Error(err, "error reconciling folder", "folder", folder.Name, "grafana", grafana.Name) applyErrors[fmt.Sprintf("%s/%s", grafana.Namespace, grafana.Name)] = err.Error() } } - condition := buildSynchronizedCondition("Folder", conditionFolderSynchronized, folder.Generation, applyErrors, len(instances.Items)) + condition := buildSynchronizedCondition("Folder", conditionFolderSynchronized, folder.Generation, applyErrors, len(instances)) meta.SetStatusCondition(&folder.Status.Conditions, condition) - if len(applyErrors) != 0 { - return ctrl.Result{RequeueAfter: RequeueDelay}, nil + if len(applyErrors) > 0 { + return ctrl.Result{RequeueAfter: RequeueDelay}, fmt.Errorf("failed to apply to all instances: %v", applyErrors) } if folder.ResyncPeriodHasElapsed() { @@ -444,20 +431,3 @@ func (r *GrafanaFolderReconciler) Exists(client *genapi.GrafanaHTTPAPI, cr *graf page++ } } - -func (r *GrafanaFolderReconciler) GetMatchingFolderInstances(ctx context.Context, folder *grafanav1beta1.GrafanaFolder, k8sClient client.Client) (grafanav1beta1.GrafanaList, error) { - instances, err := GetMatchingInstances(ctx, k8sClient, folder.Spec.InstanceSelector) - if err != nil || len(instances.Items) == 0 { - folder.Status.NoMatchingInstances = true - if err := r.Client.Status().Update(ctx, folder); err != nil { - r.Log.Info("unable to update the status of %v, in %v", folder.Name, folder.Namespace) - } - return grafanav1beta1.GrafanaList{}, err - } - folder.Status.NoMatchingInstances = false - if err := r.Client.Status().Update(ctx, folder); err != nil { - r.Log.Info("unable to update the status of %v, in %v", folder.Name, folder.Namespace) - } - - return instances, err -} diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go index d831f03eb..7fa34cf21 100644 --- a/controllers/notificationpolicy_controller.go +++ b/controllers/notificationpolicy_controller.go @@ -39,6 +39,7 @@ import ( const ( conditionNotificationPolicySynchronized = "NotificationPolicySynchronized" + annotationAppliedNotificationPolicy = "operator.grafana.com/applied-notificationpolicy" ) // GrafanaNotificationPolicyReconciler reconciles a GrafanaNotificationPolicy object