From 67221fb32819c60a048999bb6e6621be0b7ddfc7 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Tue, 4 Mar 2025 15:31:41 -0700 Subject: [PATCH] K8s: Folders: Fix not found errors (#101585) --- pkg/registry/apis/folders/legacy_storage.go | 10 +-- .../folder/folderimpl/unifiedstore.go | 31 +++++---- .../folder/folderimpl/unifiedstore_test.go | 63 +++++++++++++++++++ 3 files changed, 87 insertions(+), 17 deletions(-) diff --git a/pkg/registry/apis/folders/legacy_storage.go b/pkg/registry/apis/folders/legacy_storage.go index 34dcaa9a73b..550331c855e 100644 --- a/pkg/registry/apis/folders/legacy_storage.go +++ b/pkg/registry/apis/folders/legacy_storage.go @@ -2,7 +2,6 @@ package folders import ( "context" - "errors" "fmt" "strings" @@ -138,13 +137,14 @@ func (s *legacyStorage) Get(ctx context.Context, name string, options *metav1.Ge UID: &name, OrgID: info.OrgID, }) - if err != nil || dto == nil { - if errors.Is(err, dashboards.ErrFolderNotFound) || err == nil { - err = resourceInfo.NewNotFound(name) - } + if err != nil { statusErr := apierrors.ToFolderStatusError(err) return nil, &statusErr } + if dto == nil { + statusErr := apierrors.ToFolderStatusError(dashboards.ErrFolderNotFound) + return nil, &statusErr + } r, err := convertToK8sResource(dto, s.namespacer) if err != nil { diff --git a/pkg/services/folder/folderimpl/unifiedstore.go b/pkg/services/folder/folderimpl/unifiedstore.go index b1615eb4b1e..0d64cdcf2f3 100644 --- a/pkg/services/folder/folderimpl/unifiedstore.go +++ b/pkg/services/folder/folderimpl/unifiedstore.go @@ -77,6 +77,10 @@ func (ss *FolderUnifiedStoreImpl) Delete(ctx context.Context, UIDs []string, org func (ss *FolderUnifiedStoreImpl) Update(ctx context.Context, cmd folder.UpdateFolderCommand) (*folder.Folder, error) { obj, err := ss.k8sclient.Get(ctx, cmd.UID, cmd.OrgID, v1.GetOptions{}) if err != nil { + if apierrors.IsNotFound(err) { + return nil, dashboards.ErrFolderNotFound + } + return nil, err } updated := obj.DeepCopy() @@ -144,7 +148,7 @@ func (ss *FolderUnifiedStoreImpl) GetParents(ctx context.Context, q folder.GetPa parentUid := q.UID for parentUid != "" { - out, err := ss.k8sclient.Get(ctx, parentUid, q.OrgID, v1.GetOptions{}) + folder, err := ss.Get(ctx, folder.GetFolderQuery{UID: &parentUid, OrgID: q.OrgID}) if err != nil { var statusError *apierrors.StatusError if errors.As(err, &statusError) && statusError.ErrStatus.Code == http.StatusForbidden { @@ -155,11 +159,6 @@ func (ss *FolderUnifiedStoreImpl) GetParents(ctx context.Context, q folder.GetPa return nil, err } - folder, err := ss.UnstructuredToLegacyFolder(ctx, out) - if err != nil { - return nil, err - } - parentUid = folder.ParentUID hits = append(hits, folder) } @@ -183,6 +182,15 @@ func (ss *FolderUnifiedStoreImpl) GetChildren(ctx context.Context, q folder.GetC q.Page = 1 } + if q.UID != "" { + // the original get children query fails if the parent folder does not exist + // check that the parent exists first + _, err := ss.Get(ctx, folder.GetFolderQuery{UID: &q.UID, OrgID: q.OrgID}) + if err != nil { + return nil, err + } + } + req := &resource.ResourceSearchRequest{ Options: &resource.ListOptions{ Fields: []*resource.Requirement{ @@ -229,12 +237,7 @@ func (ss *FolderUnifiedStoreImpl) GetChildren(ctx context.Context, q folder.GetC } // search only returns a subset of info, get all info of the folder - item, err := ss.k8sclient.Get(ctx, item.Name, q.OrgID, v1.GetOptions{}) - if err != nil { - return nil, err - } - - f, err := ss.UnstructuredToLegacyFolder(ctx, item) + f, err := ss.Get(ctx, folder.GetFolderQuery{UID: &item.Name, OrgID: q.OrgID}) if err != nil { return nil, err } @@ -411,6 +414,10 @@ func getDescendants(nodes map[string]*folder.Folder, tree map[string]map[string] func (ss *FolderUnifiedStoreImpl) CountFolderContent(ctx context.Context, orgID int64, ancestor_uid string) (folder.DescendantCounts, error) { counts, err := ss.k8sclient.Get(ctx, ancestor_uid, orgID, v1.GetOptions{}, "counts") if err != nil { + if apierrors.IsNotFound(err) { + return nil, dashboards.ErrFolderNotFound + } + return nil, err } diff --git a/pkg/services/folder/folderimpl/unifiedstore_test.go b/pkg/services/folder/folderimpl/unifiedstore_test.go index f056ae96ba5..6c51829f671 100644 --- a/pkg/services/folder/folderimpl/unifiedstore_test.go +++ b/pkg/services/folder/folderimpl/unifiedstore_test.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/apiserver/client" + "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/stretchr/testify/mock" @@ -16,6 +17,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/selection" ) @@ -170,6 +172,16 @@ func TestGetParents(t *testing.T) { require.Len(t, result, 1) require.Equal(t, "parenttwo", result[0].UID) }) + t.Run("should stop if parent folder is not found", func(t *testing.T) { + mockCli.On("Get", mock.Anything, "parentone", orgID, mock.Anything, mock.Anything).Return(nil, apierrors.NewNotFound(schema.GroupResource{Group: "folders.folder.grafana.app", Resource: "folder"}, "parentone")).Once() + + _, err := store.GetParents(ctx, folder.GetParentsQuery{ + UID: "parentone", + OrgID: orgID, + }) + + require.ErrorIs(t, err, dashboards.ErrFolderNotFound) + }) } func TestGetChildren(t *testing.T) { @@ -213,6 +225,11 @@ func TestGetChildren(t *testing.T) { }, TotalHits: 1, }, nil).Once() + mockCli.On("Get", mock.Anything, "folder1", orgID, mock.Anything, mock.Anything).Return(&unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{"name": "folder1"}, + }, + }, nil).Once() mockCli.On("Get", mock.Anything, "folder2", orgID, mock.Anything, mock.Anything).Return(&unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{"name": "folder2"}, @@ -235,6 +252,47 @@ func TestGetChildren(t *testing.T) { require.Equal(t, "folder3", result[1].UID) }) + t.Run("should return an error if the folder is not found", func(t *testing.T) { + mockCli.On("Search", mock.Anything, orgID, &resource.ResourceSearchRequest{ + Options: &resource.ListOptions{ + Fields: []*resource.Requirement{ + { + Key: resource.SEARCH_FIELD_FOLDER, + Operator: string(selection.In), + Values: []string{"folder1"}, + }, + }, + }, + Limit: folderSearchLimit, // should default to folderSearchLimit + Offset: 0, // should be set as limit * (page - 1) + Page: 1, // should be set to 1 by default + }).Return(&resource.ResourceSearchResponse{ + Results: &resource.ResourceTable{ + Columns: []*resource.ResourceTableColumnDefinition{ + {Name: "folder", Type: resource.ResourceTableColumnDefinition_STRING}, + }, + Rows: []*resource.ResourceTableRow{ + { + Key: &resource.ResourceKey{Name: "folder2", Resource: "folder"}, + Cells: [][]byte{[]byte("folder1")}, + }, + { + Key: &resource.ResourceKey{Name: "folder3", Resource: "folder"}, + Cells: [][]byte{[]byte("folder1")}, + }, + }, + }, + TotalHits: 1, + }, nil).Once() + mockCli.On("Get", mock.Anything, "folder1", orgID, mock.Anything, mock.Anything).Return(nil, apierrors.NewNotFound(schema.GroupResource{Group: "folders.folder.grafana.app", Resource: "folder"}, "folder1")).Once() + + _, err := store.GetChildren(ctx, folder.GetChildrenQuery{ + UID: "folder1", + OrgID: orgID, + }) + require.ErrorIs(t, err, dashboards.ErrFolderNotFound) + }) + t.Run("pages should be able to be set, general folder should be turned to empty string, and folder uids should be passed in", func(t *testing.T) { mockCli.On("Search", mock.Anything, orgID, &resource.ResourceSearchRequest{ Options: &resource.ListOptions{ @@ -301,6 +359,11 @@ func TestGetChildren(t *testing.T) { }, TotalHits: 1, }, nil) + mockCli.On("Get", mock.Anything, "folder", orgID, mock.Anything, mock.Anything).Return(&unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{"name": "folder"}, + }, + }, nil) mockCli.On("Get", mock.Anything, accesscontrol.K6FolderUID, orgID, mock.Anything, mock.Anything).Return(&unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{"name": accesscontrol.K6FolderUID},