From d0ff3be451dc24364adfca07f28d31a59710226d Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Tue, 10 Mar 2026 18:26:53 -0600 Subject: [PATCH] Folders: Remove with full path annotation from k8s api (#119998) --- pkg/apimachinery/utils/meta.go | 6 - pkg/registry/apis/folders/conversions.go | 8 -- pkg/registry/apis/folders/legacy_storage.go | 6 - .../apis/folders/legacy_storage_test.go | 39 ----- pkg/services/folder/folderimpl/conversions.go | 5 +- .../folder/folderimpl/conversions_test.go | 4 +- .../folder/folderimpl/unifiedstore.go | 9 +- .../folder/folderimpl/unifiedstore_test.go | 136 ------------------ pkg/storage/unified/apistore/util.go | 11 +- pkg/storage/unified/apistore/util_test.go | 27 ---- pkg/storage/unified/resource/server.go | 2 +- 11 files changed, 8 insertions(+), 245 deletions(-) diff --git a/pkg/apimachinery/utils/meta.go b/pkg/apimachinery/utils/meta.go index 827fe93dfe6..9acd4c5c09f 100644 --- a/pkg/apimachinery/utils/meta.go +++ b/pkg/apimachinery/utils/meta.go @@ -65,12 +65,6 @@ const AnnoKeySourcePath = "grafana.app/sourcePath" const AnnoKeySourceChecksum = "grafana.app/sourceChecksum" const AnnoKeySourceTimestamp = "grafana.app/sourceTimestamp" -// Only used in modes 0-2 (legacy db) for returning the folder fullpath - -const LabelGetFullpath = "grafana.app/fullpath" -const AnnoKeyFullpath = "grafana.app/fullpath" -const AnnoKeyFullpathUIDs = "grafana.app/fullpathUIDs" - // LabelKeyDeprecatedInternalID gives the deprecated internal ID of a resource // Deprecated: will be removed in grafana 13 const LabelKeyDeprecatedInternalID = "grafana.app/deprecatedInternalID" diff --git a/pkg/registry/apis/folders/conversions.go b/pkg/registry/apis/folders/conversions.go index 2b29a80204c..cfc371ded07 100644 --- a/pkg/registry/apis/folders/conversions.go +++ b/pkg/registry/apis/folders/conversions.go @@ -81,14 +81,6 @@ func convertToK8sResource(v *folder.Folder, namespacer request.NamespaceMapper) // We're going to have to align with that. For now we do need the user ID because the folder type stores it // as the only user identifier - if v.Fullpath != "" { - meta.SetAnnotation(utils.AnnoKeyFullpath, v.Fullpath) - } - - if v.FullpathUIDs != "" { - meta.SetAnnotation(utils.AnnoKeyFullpathUIDs, v.FullpathUIDs) - } - if v.CreatedBy != 0 { meta.SetCreatedBy(claims.NewTypeID(claims.TypeUser, strconv.FormatInt(v.CreatedBy, 10))) } diff --git a/pkg/registry/apis/folders/legacy_storage.go b/pkg/registry/apis/folders/legacy_storage.go index d2a2c4fdfac..f669c7aa6f7 100644 --- a/pkg/registry/apis/folders/legacy_storage.go +++ b/pkg/registry/apis/folders/legacy_storage.go @@ -6,7 +6,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/registry/rest" @@ -94,11 +93,6 @@ func (s *legacyStorage) List(ctx context.Context, options *internalversion.ListO query.Limit = paging.limit query.Page = paging.page - if options.LabelSelector != nil && options.LabelSelector.Matches(labels.Set{utils.LabelGetFullpath: "true"}) { - query.WithFullpath = true - query.WithFullpathUIDs = true - } - hits, err := s.service.GetFoldersLegacy(ctx, query) if err != nil { return nil, err diff --git a/pkg/registry/apis/folders/legacy_storage_test.go b/pkg/registry/apis/folders/legacy_storage_test.go index b755f04e2e2..53d4c3109c5 100644 --- a/pkg/registry/apis/folders/legacy_storage_test.go +++ b/pkg/registry/apis/folders/legacy_storage_test.go @@ -9,11 +9,9 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" - "k8s.io/apimachinery/pkg/labels" folderv1 "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1beta1" "github.com/grafana/grafana/pkg/apimachinery/identity" - "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/foldertest" "github.com/grafana/grafana/pkg/services/org" @@ -180,41 +178,4 @@ func TestLegacyStorage_List_LabelSelector(t *testing.T) { require.True(t, ok) require.Len(t, list.Items, 1) }) - - t.Run("should set fullpath query parameters when label selector matches", func(t *testing.T) { - selector, err := labels.Parse(utils.LabelGetFullpath + "=true") - require.NoError(t, err) - options := &metainternalversion.ListOptions{ - LabelSelector: selector, - } - - folders := []*folder.Folder{ - { - UID: "folder-1", - Title: "Folder 1", - Fullpath: "/Folder 1", - FullpathUIDs: "/folder-1", - }, - } - folderService.ExpectedFolders = folders - - result, err := storage.List(ctx, options) - require.NoError(t, err) - - // verify we queried the service correctly - require.True(t, folderService.LastQuery.WithFullpath) - require.True(t, folderService.LastQuery.WithFullpathUIDs) - - list, ok := result.(*folderv1.FolderList) - require.True(t, ok) - require.Len(t, list.Items, 1) - - folder := list.Items[0] - meta, err := utils.MetaAccessor(&folder) - require.NoError(t, err) - - // make sure the annotations are set - require.Equal(t, "/Folder 1", meta.GetAnnotation(utils.AnnoKeyFullpath)) - require.Equal(t, "/folder-1", meta.GetAnnotation(utils.AnnoKeyFullpathUIDs)) - }) } diff --git a/pkg/services/folder/folderimpl/conversions.go b/pkg/services/folder/folderimpl/conversions.go index 74f1ec63b11..3ab3aaaa7e2 100644 --- a/pkg/services/folder/folderimpl/conversions.go +++ b/pkg/services/folder/folderimpl/conversions.go @@ -72,8 +72,9 @@ func convertUnstructuredToFolder(item *unstructured.Unstructured, identifiers ma Version: int(meta.GetGeneration()), ManagedBy: manager.Kind, - Fullpath: meta.GetAnnotation(utils.AnnoKeyFullpath), - FullpathUIDs: meta.GetAnnotation(utils.AnnoKeyFullpathUIDs), + // Fullpath/FullpathUIDs are not stored on objects; callers use buildFolderFullPaths when needed + Fullpath: "", + FullpathUIDs: "", URL: url, Created: created, Updated: *updated, diff --git a/pkg/services/folder/folderimpl/conversions_test.go b/pkg/services/folder/folderimpl/conversions_test.go index 11376ebd8ef..f1506bf5330 100644 --- a/pkg/services/folder/folderimpl/conversions_test.go +++ b/pkg/services/folder/folderimpl/conversions_test.go @@ -103,9 +103,7 @@ func TestFolderListConversions(t *testing.T) { "creationTimestamp": "2022-12-02T02:02:02Z", "generation": 1, "labels": { - "grafana.app/deprecatedInternalID": "4", - "grafana.app/fullpath": "somefullpath", - "grafana.app/fullpathUIDs": "somefullpathuids" + "grafana.app/deprecatedInternalID": "4" }, "name": "foldername1", "namespace": "default", diff --git a/pkg/services/folder/folderimpl/unifiedstore.go b/pkg/services/folder/folderimpl/unifiedstore.go index 457f4dd97b1..7215a8eb9a6 100644 --- a/pkg/services/folder/folderimpl/unifiedstore.go +++ b/pkg/services/folder/folderimpl/unifiedstore.go @@ -353,14 +353,7 @@ func (ss *FolderUnifiedStoreImpl) GetFolders(ctx context.Context, q folder.GetFo ctx, span := ss.tracer.Start(ctx, tracePrefix+"GetFolders") defer span.End() - opts := v1.ListOptions{} - if q.WithFullpath || q.WithFullpathUIDs { - // only supported in modes 0-2, to keep the alerting queries from causing tons of get folder requests - // to retrieve the parent for all folders in grafana - opts.LabelSelector = utils.LabelGetFullpath + "=true" - } - - out, err := ss.list(ctx, q.OrgID, opts) + out, err := ss.list(ctx, q.OrgID, v1.ListOptions{}) if err != nil { return nil, err } diff --git a/pkg/services/folder/folderimpl/unifiedstore_test.go b/pkg/services/folder/folderimpl/unifiedstore_test.go index 6dc8379a1a8..030d2e6693d 100644 --- a/pkg/services/folder/folderimpl/unifiedstore_test.go +++ b/pkg/services/folder/folderimpl/unifiedstore_test.go @@ -583,142 +583,6 @@ func TestGetFolders(t *testing.T) { }, wantErr: false, }, - { - name: "should return all folders from k8s with fullpath enabled", - args: args{ - ctx: context.Background(), - q: folder.GetFoldersFromStoreQuery{ - GetFoldersQuery: folder.GetFoldersQuery{ - OrgID: orgID, - WithFullpath: true, - }, - }, - }, - mock: func(mockCli *client.MockK8sHandler) { - mockCli.On("List", mock.Anything, orgID, metav1.ListOptions{ - Limit: folderListLimit, - TypeMeta: metav1.TypeMeta{}, - LabelSelector: "grafana.app/fullpath=true", - }).Return(&unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "root1", - "uid": "root1", - }, - "spec": map[string]interface{}{ - "title": "root1", - }, - }, - }, - { - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "root2", - "uid": "root2", - }, - "spec": map[string]interface{}{ - "title": "root2", - }, - }, - }, - }, - }, nil).Once() - }, - want: []*folder.Folder{ - { - UID: "root1", - Title: "root1", - OrgID: orgID, - Fullpath: "root1", - FullpathUIDs: "root1", - }, - { - UID: "root2", - Title: "root2", - OrgID: orgID, - Fullpath: "root2", - FullpathUIDs: "root2", - }, - }, - wantErr: false, - }, - { - name: "should return folders from k8s by uid with fullpath enabled", - args: args{ - ctx: context.Background(), - q: folder.GetFoldersFromStoreQuery{ - GetFoldersQuery: folder.GetFoldersQuery{ - OrgID: orgID, - UIDs: []string{"folder1", "folder2"}, - WithFullpath: true, - }, - }, - }, - mock: func(mockCli *client.MockK8sHandler) { - mockCli.On("List", mock.Anything, orgID, metav1.ListOptions{ - Limit: folderListLimit, - TypeMeta: metav1.TypeMeta{}, - LabelSelector: "grafana.app/fullpath=true", - }).Return(&unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "parentcommon", - "uid": "parentcommon", - }, - "spec": map[string]interface{}{ - "title": "parentcommon", - }, - }, - }, - { - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "folder1", - "uid": "folder1", - "annotations": map[string]interface{}{"grafana.app/folder": "parentcommon"}, - }, - "spec": map[string]interface{}{ - "title": "folder1", - }, - }, - }, - { - Object: map[string]interface{}{ - "metadata": map[string]interface{}{ - "name": "folder2", - "uid": "folder2", - "annotations": map[string]interface{}{"grafana.app/folder": "parentcommon"}, - }, - "spec": map[string]interface{}{ - "title": "folder2", - }, - }, - }, - }, - }, nil).Once() - }, - want: []*folder.Folder{ - { - UID: "folder1", - Title: "folder1", - OrgID: orgID, - Fullpath: "parentcommon/folder1", - FullpathUIDs: "parentcommon/folder1", - }, - { - UID: "folder2", - Title: "folder2", - OrgID: orgID, - Fullpath: "parentcommon/folder2", - FullpathUIDs: "parentcommon/folder2", - }, - }, - wantErr: false, - }, { name: "should return error if k8s returns error", args: args{ diff --git a/pkg/storage/unified/apistore/util.go b/pkg/storage/unified/apistore/util.go index 6cb4c5b31f8..ce358e47789 100644 --- a/pkg/storage/unified/apistore/util.go +++ b/pkg/storage/unified/apistore/util.go @@ -58,15 +58,8 @@ func toListRequest(k *resourcepb.ResourceKey, opts storage.ListOptions) (*resour for _, r := range requirements { v := r.Key() - // Parse the history request from labels - // TODO: for LabelGetFullpath, we just skip this for unistore. We need a better solution for - // getting the full path for folders in unistore, without making a request for each parent folder. - // In modes 0-2 we added this label to indicate that the sql query should return that data as - // an annotation on the folder. However, this annotation cannot be saved to unified storage, otherwise - // we will have to recompute annotations for all descendants of a folder during a folder move. - // While we look for a better solution, unified storage will continue to return all folders & the folder - // service will get the full path by retrieving each parent folder. - if v == utils.LabelKeyGetHistory || v == utils.LabelKeyGetTrash || v == utils.LabelGetFullpath { + // Parse the history/trash request from labels + if v == utils.LabelKeyGetHistory || v == utils.LabelKeyGetTrash { if len(requirements) != 1 { return nil, predicate, apierrors.NewBadRequest("single label supported with: " + v) } diff --git a/pkg/storage/unified/apistore/util_test.go b/pkg/storage/unified/apistore/util_test.go index 68ca1ad462f..36c82eeef04 100644 --- a/pkg/storage/unified/apistore/util_test.go +++ b/pkg/storage/unified/apistore/util_test.go @@ -218,33 +218,6 @@ func TestToListRequest(t *testing.T) { wantPredicate: storage.Everything, wantErr: nil, }, - { - name: "with fullpath label", - key: &resourcepb.ResourceKey{ - Group: "test", - Resource: "test", - Namespace: "default", - }, - opts: storage.ListOptions{ - Predicate: storage.SelectionPredicate{ - Label: labels.SelectorFromSet(labels.Set{utils.LabelGetFullpath: "true"}), - }, - }, - want: &resourcepb.ListRequest{ - VersionMatchV2: 1, - Options: &resourcepb.ListOptions{ - Labels: nil, - Fields: nil, - Key: &resourcepb.ResourceKey{ - Group: "test", - Resource: "test", - Namespace: "default", - }, - }, - }, - wantPredicate: storage.Everything, - wantErr: nil, - }, } for _, tt := range tests { diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 40d79cd0f2c..b485c2a5a75 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -559,7 +559,7 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *resour // Make sure the command labels are not saved for k := range obj.GetLabels() { - if k == utils.LabelKeyGetHistory || k == utils.LabelKeyGetTrash || k == utils.LabelGetFullpath { + if k == utils.LabelKeyGetHistory || k == utils.LabelKeyGetTrash { return nil, NewBadRequestError("can not save label: " + k) } }