merge main

pkg/registry/apis/dashboard/register.go

@@ -54,6 +54,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/dashboardsnapshots"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/grafana/grafana/pkg/services/folder"
 	"github.com/grafana/grafana/pkg/services/libraryelements"
 	"github.com/grafana/grafana/pkg/services/librarypanels"
 	"github.com/grafana/grafana/pkg/services/live"
@@ -388,8 +389,12 @@ func (b *DashboardsAPIBuilder) validateCreate(ctx context.Context, a admission.A
 		return fmt.Errorf("error getting requester: %w", err)
 	}
 
+	if a.IsDryRun() {
+		return nil // do not check folder or quota
+	}
+
 	// Validate folder existence if specified
-	if !a.IsDryRun() && accessor.GetFolder() != "" {
+	if !folder.IsRootFolder(accessor.GetFolder()) {
 		folder, err := b.validateFolderExists(ctx, accessor.GetFolder(), id.GetOrgID())
 		if err != nil {
 			return err
@@ -401,7 +406,7 @@ func (b *DashboardsAPIBuilder) validateCreate(ctx context.Context, a admission.A
 	}
 
 	// Validate quota
-	if !b.isStandalone && !a.IsDryRun() {
+	if !b.isStandalone {
 		params := &quota.ScopeParameters{}
 		params.OrgID = id.GetOrgID()
 		internalId, err := id.GetInternalID()

pkg/registry/apis/folders/validate.go

@@ -46,12 +46,12 @@ func validateOnCreate(ctx context.Context, f *folders.Folder, getter parentsGett
 		return dashboards.ErrFolderTitleEmpty
 	}
 
-	parentName := meta.GetFolder()
-	if parentName == "" {
+	switch meta.GetFolder() {
+	case "", folder.GeneralFolderUID:
 		return nil // OK, we do not need to validate the tree
-	}
-
-	if parentName == f.Name {
+	case folder.SharedWithMeFolderUID:
+		return fmt.Errorf("can not save shared with me")
+	case f.Name:
 		return folder.ErrFolderCannotBeParentOfItself
 	}
 
@@ -98,17 +98,19 @@ func validateOnUpdate(ctx context.Context,
 	// Validate the move operation
 	newParent := folderObj.GetFolder()
 
+	switch newParent {
 	// If we move to root, we don't need to validate the depth, because the folder already existed
 	// before and wasn't too deep. This move will make it more shallow.
 	//
 	// We also don't need to validate circular references because the root folder cannot have a parent.
-	if newParent == folder.RootFolderUID {
-		return nil
-	}
-
-	// folder cannot be moved to a k6 folder
-	if newParent == accesscontrol.K6FolderUID {
+	case "", folder.GeneralFolderUID:
+		return nil // OK, we do not need to validate the tree
+	case folder.SharedWithMeFolderUID:
+		return fmt.Errorf("can not save shared with me")
+	case accesscontrol.K6FolderUID:
 		return fmt.Errorf("k6 project may not be moved")
+	case folderObj.GetName():
+		return folder.ErrFolderCannotBeParentOfItself
 	}
 
 	parentObj, err := getter.Get(ctx, newParent, &metav1.GetOptions{})

pkg/services/dashboards/service/dashboard_service.go

@@ -2344,13 +2344,18 @@ func (dr *DashboardServiceImpl) unstructuredToLegacyDashboardWithUsers(item *uns
 	dashVersion := obj.GetGeneration()
 	spec["version"] = dashVersion
 
+	folderUID := obj.GetFolder()
+	if folderUID == folder.GeneralFolderUID {
+		folderUID = "" // empty in legacy API
+	}
+
 	title, _, _ := unstructured.NestedString(spec, "title")
 	out := dashboards.Dashboard{
 		OrgID:      orgID,
 		ID:         obj.GetDeprecatedInternalID(), // nolint:staticcheck
 		UID:        uid,
 		Slug:       slugify.Slugify(title),
-		FolderUID:  obj.GetFolder(),
+		FolderUID:  folderUID,
 		Version:    int(dashVersion),
 		Data:       simplejson.NewFromAny(spec),
 		APIVersion: strings.TrimPrefix(item.GetAPIVersion(), dashboardv0.GROUP+"/"),

pkg/services/folder/folderimpl/conversions.go

@@ -32,7 +32,7 @@ func convertUnstructuredToFolder(item *unstructured.Unstructured, identifiers ma
 
 	uid := meta.GetName()
 	url := ""
-	if uid != folder.RootFolder.UID {
+	if !folder.IsRootFolder(uid) {
 		slug := slugify.Slugify(title)
 		url = dashboards.GetFolderURL(uid, slug)
 	}
@@ -62,13 +62,18 @@ func convertUnstructuredToFolder(item *unstructured.Unstructured, identifiers ma
 		}
 	}
 
+	parent := meta.GetFolder()
+	if folder.IsRootFolder(parent) {
+		parent = ""
+	}
+
 	manager, _ := meta.GetManagerProperties()
 	return &folder.Folder{
 		UID:         uid,
 		Title:       title,
 		Description: description,
 		ID:          meta.GetDeprecatedInternalID(), // nolint:staticcheck
-		ParentUID:   meta.GetFolder(),
+		ParentUID:   parent,
 		Version:     int(meta.GetGeneration()),
 		ManagedBy:   manager.Kind,
 

pkg/services/folder/folderimpl/folder.go

@@ -1283,6 +1283,10 @@ func (s *Service) buildSaveDashboardCommand(ctx context.Context, dto *dashboards
 		return nil, dashboards.ErrDashboardFolderNameExists
 	}
 
+	if dash.FolderUID == folder.GeneralFolderUID {
+		dash.FolderUID = "" // general is the same as root
+	}
+
 	if dash.FolderUID != "" {
 		if _, err := s.dashboardFolderStore.GetFolderByUID(ctx, dash.OrgID, dash.FolderUID); err != nil {
 			return nil, err
@@ -1380,7 +1384,7 @@ func SplitFullpath(s string) []string {
 func (s *Service) nestedFolderCreate(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) {
 	ctx, span := s.tracer.Start(ctx, "folder.nestedFolderCreate")
 	defer span.End()
-	if cmd.ParentUID != "" {
+	if !folder.IsRootFolder(cmd.ParentUID) {
 		if err := s.validateParent(ctx, cmd.OrgID, cmd.ParentUID, cmd.UID); err != nil {
 			return nil, err
 		}

pkg/services/folder/folderimpl/unifiedstore.go

@@ -180,7 +180,7 @@ func (ss *FolderUnifiedStoreImpl) GetParents(ctx context.Context, q folder.GetPa
 	hits := []*folder.Folder{}
 
 	parentUID := q.UID
-	for parentUID != "" {
+	for !folder.IsRootFolder(parentUID) {
 		folder, err := ss.Get(ctx, folder.GetFolderQuery{UID: &parentUID, OrgID: q.OrgID})
 		if err != nil {
 			if apierrors.IsForbidden(err) {

pkg/services/folder/model.go

@@ -31,6 +31,10 @@ const (
 	SharedWithMeFolderUID = "sharedwithme"
 )
 
+func IsRootFolder(f string) bool {
+	return f == "" || f == GeneralFolderUID
+}
+
 var ErrFolderNotFound = errutil.NotFound("folder.notFound")
 
 type Folder struct {

pkg/storage/unified/apistore/prepare.go

@@ -76,6 +76,20 @@ func (v *objectForStorage) finish(ctx context.Context, err error, secrets secret
 	return nil
 }
 
+func (s *Storage) verifyFolder(obj utils.GrafanaMetaAccessor) error {
+	if s.opts.EnableFolderSupport {
+		if obj.GetFolder() == "" {
+			// return apierrors.NewBadRequest("missing folder annotation")
+			// TODO?: should this be optionally be done in a mutation webhook?
+			// ???? obj.SetFolder(folder.GeneralFolderUID) // always enter something
+			return nil
+		}
+	} else if obj.GetFolder() != "" {
+		return apierrors.NewBadRequest("folders not supported in this resource")
+	}
+	return nil
+}
+
 // Called on create
 func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime.Object) (objectForStorage, error) {
 	v := objectForStorage{}
@@ -103,6 +117,9 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime
 	if s.opts.MaximumNameLength > 0 && len(obj.GetName()) > s.opts.MaximumNameLength {
 		return v, apierrors.NewBadRequest(fmt.Sprintf("name exceeds maximum length (%d)", s.opts.MaximumNameLength))
 	}
+	if err = s.verifyFolder(obj); err != nil {
+		return v, err
+	}
 
 	v.grantPermissions = obj.GetAnnotation(utils.AnnoKeyGrantPermissions)
 	if v.grantPermissions != "" {
@@ -193,17 +210,15 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti
 		obj.SetDeprecatedInternalID(previousInternalID) // nolint:staticcheck
 	}
 
-	err = prepareSecureValues(ctx, s.opts.SecureValues, obj, previous, &v)
-	if err != nil {
+	if err = prepareSecureValues(ctx, s.opts.SecureValues, obj, previous, &v); err != nil {
+		return v, err
+	}
+	if err = s.verifyFolder(obj); err != nil {
 		return v, err
 	}
 
 	// Check if we should bump the generation
 	if obj.GetFolder() != previous.GetFolder() {
-		if !s.opts.EnableFolderSupport {
-			return v, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String()))
-		}
-		// TODO: check that we can move the folder?
 		v.hasChanged = true
 	} else if obj.GetDeletionTimestamp() != nil && previous.GetDeletionTimestamp() == nil {
 		v.hasChanged = true // bump generation when deleted

pkg/storage/unified/apistore/store.go

@@ -209,6 +209,13 @@ func (s *Storage) convertToObject(ctx context.Context, data []byte, obj runtime.
 	_, span := tracer.Start(ctx, "apistore.Storage.convertToObject")
 	defer span.End()
 	obj, _, err := s.codec.Decode(data, nil, obj)
+	// TODO!!! Replace empty folder with "general" on read (this was not a requirement early on)
+	// if s.opts.EnableFolderSupport {
+	// 	m, _ := utils.MetaAccessor(obj)
+	// 	if m != nil && m.GetFolder() == "" {
+	// 		m.SetFolder(folder.GeneralFolderUID)
+	// 	}
+	// }
 	return obj, err
 }
 

pkg/tests/api/dashboards/api_dashboards_test.go

@@ -1166,13 +1166,13 @@ func TestIntegrationDashboardServicePermissions(t *testing.T) {
 
 		resp, err := postDashboard(t, grafanaListedAddr, "viewer", "viewer", dashboardPayload)
 		require.NoError(t, err)
-		assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+		require.Equal(t, http.StatusForbidden, resp.StatusCode)
 		err = resp.Body.Close()
 		require.NoError(t, err)
 
 		resp, err = postDashboard(t, grafanaListedAddr, "editor", "editor", dashboardPayload)
 		require.NoError(t, err)
-		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		require.Equal(t, http.StatusOK, resp.StatusCode)
 		err = resp.Body.Close()
 		require.NoError(t, err)
 	})

pkg/tests/apis/folder/folder_tree_test.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/xlab/treeprint"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -377,7 +378,7 @@ func getFoldersFromLegacyAPISearch(t *testing.T, client *rest.RESTClient) *Folde
 		result = client.Get().AbsPath("api", "folders", hit.UID).
 			Do(context.Background()).
 			StatusCode(&statusCode)
-		require.NoError(t, result.Error(), "getting folder access info (/api)")
+		assert.NoError(t, result.Error(), "getting folder access info (/api) uid:%s", hit.UID)
 		require.Equal(t, int(http.StatusOK), statusCode)
 
 		body, err := result.Raw()
@@ -394,7 +395,7 @@ func makeRoot(lookup map[string]*FolderView, name string) *FolderView {
 	shared := &FolderView{} // when not found
 	root := &FolderView{}
 	for _, v := range lookup {
-		if v.Parent == "" {
+		if folder.IsRootFolder(v.Parent) { // general or empty
 			root.Children = append(root.Children, v)
 		} else {
 			p, ok := lookup[v.Parent]
@@ -445,7 +446,7 @@ func getFoldersFromDashboardV0Search(t *testing.T, client *rest.RESTClient, ns s
 			folderV1.APIVersion, "namespaces", ns, "folders", hit.Name, "access").
 			Do(context.Background()).
 			StatusCode(&statusCode)
-		require.NoError(t, result.Error(), "getting folder access info (/access)")
+		require.NoError(t, result.Error(), "getting folder access info (/access) name:%s", hit.Name)
 		require.Equal(t, int(http.StatusOK), statusCode)
 
 		body, err := result.Raw()

pkg/tests/apis/folder/folders_test.go

@@ -133,10 +133,14 @@ func TestIntegrationFoldersApp(t *testing.T) {
 		}`, string(v1Disco))
 	})
 
-	// test on all dualwriter modes
-	for mode := 0; mode <= 4; mode++ {
-		modeDw := grafanarest.DualWriterMode(mode)
-
+	// test on all dual writer modes
+	modes := []grafanarest.DualWriterMode{
+		grafanarest.Mode0, // legacy only
+		grafanarest.Mode2, // write both, read legacy
+		grafanarest.Mode3, // write both, read unified
+		grafanarest.Mode4,
+	}
+	for _, modeDw := range modes {
 		t.Run(fmt.Sprintf("with dual write (unified storage, mode %v)", modeDw), func(t *testing.T) {
 			doFolderTests(t, apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{
 				DisableDataMigrations: true,
@@ -1257,11 +1261,13 @@ func TestIntegrationFoldersGetAPIEndpointK8S(t *testing.T) {
 			requestToAnotherOrg: true,
 		},
 	}
-
-	for mode := 0; mode <= 4; mode++ {
-		t.Run(fmt.Sprintf("Mode_%d", mode), func(t *testing.T) {
-			modeDw := grafanarest.DualWriterMode(mode)
-
+	for _, modeDw := range []grafanarest.DualWriterMode{
+		grafanarest.Mode0, // legacy only
+		grafanarest.Mode2, // write both, read legacy
+		grafanarest.Mode3, // write both, read unified
+		grafanarest.Mode4,
+	} {
+		t.Run(fmt.Sprintf("Mode_%d", modeDw), func(t *testing.T) {
 			helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{
 				DisableDataMigrations: true,
 				AppModeProduction:     true,