From 378bb6ea3fb9965a3467657ba7262cc4d2398502 Mon Sep 17 00:00:00 2001 From: Scott Lepper Date: Fri, 7 Feb 2025 17:19:23 -0500 Subject: [PATCH] [search] folder name lookup performance (#100154) [search] use search for folder name lookup --- pkg/api/apierrors/folder.go | 4 ++ pkg/api/dashboard.go | 12 ++++ pkg/registry/apis/dashboard/search.go | 2 +- .../dashboards/service/dashboard_service.go | 64 ++++++++++++------- .../service/dashboard_service_test.go | 17 ++++- .../dashboards/service/search/search.go | 12 ++-- 6 files changed, 80 insertions(+), 31 deletions(-) diff --git a/pkg/api/apierrors/folder.go b/pkg/api/apierrors/folder.go index 531636bc9c9..b29d5000fad 100644 --- a/pkg/api/apierrors/folder.go +++ b/pkg/api/apierrors/folder.go @@ -88,3 +88,7 @@ func ToFolderStatusError(err error) k8sErrors.StatusError { }, } } + +func IsForbidden(err error) bool { + return k8sErrors.IsForbidden(err) || errors.Is(err, dashboards.ErrFolderAccessDenied) +} diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index 5079d7d9877..013dbebb5eb 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -201,6 +201,18 @@ func (hs *HTTPServer) GetDashboard(c *contextmodel.ReqContext) response.Response if errors.Is(err, dashboards.ErrFolderNotFound) { return response.Error(http.StatusNotFound, "Folder not found", err) } + if apierrors.IsForbidden(err) { + // the dashboard is in a folder the user can't access, so return the dashboard without folder info + err = nil + queryResult = &folder.Folder{ + UID: dash.FolderUID, + } + } + if err != nil { + hs.log.Error("Failed to get dashboard folder", "error", err) + return response.Error(http.StatusInternalServerError, "Dashboard folder could not be read", err) + } + meta.FolderUid = queryResult.UID meta.FolderTitle = queryResult.Title meta.FolderId = queryResult.ID // nolint:staticcheck diff --git a/pkg/registry/apis/dashboard/search.go b/pkg/registry/apis/dashboard/search.go index 111b872cecc..824243692a1 100644 --- a/pkg/registry/apis/dashboard/search.go +++ b/pkg/registry/apis/dashboard/search.go @@ -351,7 +351,7 @@ func (s *SearchHandler) DoSearch(w http.ResponseWriter, r *http.Request) { return } - if parsedResults != nil && len(searchRequest.SortBy) == 0 { + if len(searchRequest.SortBy) == 0 { // default sort by resource descending ( folders then dashboards ) then title sort.Slice(parsedResults.Hits, func(i, j int) bool { return parsedResults.Hits[i].Resource > parsedResults.Hits[j].Resource || diff --git a/pkg/services/dashboards/service/dashboard_service.go b/pkg/services/dashboards/service/dashboard_service.go index 95d6c8af652..9508f9711e6 100644 --- a/pkg/services/dashboards/service/dashboard_service.go +++ b/pkg/services/dashboards/service/dashboard_service.go @@ -14,6 +14,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/otel" + "golang.org/x/exp/maps" "golang.org/x/exp/slices" "golang.org/x/sync/errgroup" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -1248,26 +1249,13 @@ func (dr *DashboardServiceImpl) FindDashboards(ctx context.Context, query *dashb return nil, err } + folderNames, err := dr.fetchFolderNames(ctx, query, response.Hits) + if err != nil { + return nil, err + } + finalResults := make([]dashboards.DashboardSearchProjection, len(response.Hits)) - // Create a small runtime cache for folders to avoid extra calls to the folder service - foldersMap := make(map[string]*folder.Folder) - serviceCtx, serviceIdent := identity.WithServiceIdentity(ctx, query.OrgId) for i, hit := range response.Hits { - f, ok := foldersMap[hit.Folder] - if !ok { - // We can get search result where user don't have access to parents. If that happens this thi - // will fail if we call it as the requesting user. To resolve this we call this as the service so we can - // garantuee that we can fetch the parent. - f, err = dr.folderService.Get(serviceCtx, &folder.GetFolderQuery{ - UID: &hit.Folder, - OrgID: query.OrgId, - SignedInUser: serviceIdent, - }) - if err != nil { - return nil, err - } - foldersMap[hit.Folder] = f - } result := dashboards.DashboardSearchProjection{ ID: hit.Field.GetNestedInt64(search.DASHBOARD_LEGACY_ID), UID: hit.Name, @@ -1276,7 +1264,7 @@ func (dr *DashboardServiceImpl) FindDashboards(ctx context.Context, query *dashb Slug: slugify.Slugify(hit.Title), IsFolder: false, FolderUID: hit.Folder, - FolderTitle: f.Title, + FolderTitle: folderNames[hit.Folder], Tags: hit.Tags, } @@ -1293,6 +1281,28 @@ func (dr *DashboardServiceImpl) FindDashboards(ctx context.Context, query *dashb return dr.dashboardStore.FindDashboards(ctx, query) } +func (dr *DashboardServiceImpl) fetchFolderNames(ctx context.Context, query *dashboards.FindPersistedDashboardsQuery, hits []dashboardv0alpha1.DashboardHit) (map[string]string, error) { + // call this with elevated permissions so we can get folder names where user does not have access + // some dashboards are shared directly with user, but the folder is not accessible via the folder permissions + serviceCtx, serviceIdent := identity.WithServiceIdentity(ctx, query.OrgId) + search := folder.SearchFoldersQuery{ + UIDs: getFolderUIDs(hits), + OrgID: query.OrgId, + SignedInUser: serviceIdent, + } + + folders, err := dr.folderService.SearchFolders(serviceCtx, search) + if err != nil { + return nil, folder.ErrInternal.Errorf("failed to fetch parent folders: %w", err) + } + + folderNames := make(map[string]string) + for _, f := range folders { + folderNames[f.UID] = f.Title + } + return folderNames, nil +} + func (dr *DashboardServiceImpl) SearchDashboards(ctx context.Context, query *dashboards.FindPersistedDashboardsQuery) (model.HitList, error) { ctx, span := tracer.Start(ctx, "dashboards.service.SearchDashboards") defer span.End() @@ -1652,7 +1662,7 @@ func (dr *DashboardServiceImpl) listDashboardsThroughK8s(ctx context.Context, or return dashboards, nil } -func (dr *DashboardServiceImpl) searchDashboardsThroughK8sRaw(ctx context.Context, query *dashboards.FindPersistedDashboardsQuery) (*dashboardv0alpha1.SearchResults, error) { +func (dr *DashboardServiceImpl) searchDashboardsThroughK8sRaw(ctx context.Context, query *dashboards.FindPersistedDashboardsQuery) (dashboardv0alpha1.SearchResults, error) { request := &resource.ResourceSearchRequest{ Options: &resource.ListOptions{ Fields: []*resource.Requirement{}, @@ -1777,7 +1787,7 @@ func (dr *DashboardServiceImpl) searchDashboardsThroughK8sRaw(ctx context.Contex } if err != nil { - return nil, err + return dashboardv0alpha1.SearchResults{}, err } if federate != nil { @@ -1794,7 +1804,7 @@ func (dr *DashboardServiceImpl) searchDashboardsThroughK8sRaw(ctx context.Contex res, err := dr.k8sclient.Search(ctx, query.OrgId, request) if err != nil { - return nil, err + return dashboardv0alpha1.SearchResults{}, err } return dashboardsearch.ParseResults(res, 0) @@ -2083,3 +2093,13 @@ func LegacySaveCommandToUnstructured(cmd *dashboards.SaveDashboardCommand, names return finalObj, nil } + +func getFolderUIDs(hits []dashboardv0alpha1.DashboardHit) []string { + folderSet := map[string]bool{} + for _, hit := range hits { + if hit.Folder != "" && !folderSet[hit.Folder] { + folderSet[hit.Folder] = true + } + } + return maps.Keys(folderSet) +} diff --git a/pkg/services/dashboards/service/dashboard_service_test.go b/pkg/services/dashboards/service/dashboard_service_test.go index 5e9167c1f50..ccfa8f9e3a3 100644 --- a/pkg/services/dashboards/service/dashboard_service_test.go +++ b/pkg/services/dashboards/service/dashboard_service_test.go @@ -1338,7 +1338,9 @@ func TestSearchDashboards(t *testing.T) { fakeFolders := foldertest.NewFakeService() fakeFolders.ExpectedFolder = &folder.Folder{ Title: "testing-folder-1", + UID: "f1", } + fakeFolders.ExpectedFolders = []*folder.Folder{fakeFolders.ExpectedFolder} defer fakeStore.AssertExpectations(t) service := &DashboardServiceImpl{ cfg: setting.NewCfg(), @@ -1359,6 +1361,7 @@ func TestSearchDashboards(t *testing.T) { "tag2", }, FolderTitle: "testing-folder-1", + FolderUID: "f1", }, { UID: "uid2", @@ -1369,6 +1372,7 @@ func TestSearchDashboards(t *testing.T) { URL: "/d/uid2/dashboard-2", Tags: []string{}, FolderTitle: "testing-folder-1", + FolderUID: "f1", }, } query := dashboards.FindPersistedDashboardsQuery{ @@ -1384,6 +1388,7 @@ func TestSearchDashboards(t *testing.T) { Title: "Dashboard 1", Tags: []string{"tag1", "tag2"}, FolderTitle: "testing-folder-1", + FolderUID: "f1", }, { UID: "uid2", @@ -1391,6 +1396,7 @@ func TestSearchDashboards(t *testing.T) { OrgID: 1, Title: "Dashboard 2", FolderTitle: "testing-folder-1", + FolderUID: "f1", }, }, nil).Once() result, err := service.SearchDashboards(context.Background(), &query) @@ -1401,6 +1407,13 @@ func TestSearchDashboards(t *testing.T) { t.Run("Should use Kubernetes client if feature flags are enabled", func(t *testing.T) { ctx, k8sCliMock := setupK8sDashboardTests(service) + expectedFolders := model.HitList{ + { + UID: "f1", + Title: "testing-folder-1", + }, + } + fakeFolders.ExpectedHitList = expectedFolders k8sCliMock.On("GetNamespace", mock.Anything, mock.Anything).Return("default") k8sCliMock.On("Search", mock.Anything, mock.Anything, mock.Anything).Return(&resource.ResourceSearchResponse{ Results: &resource.ResourceTable{ @@ -1426,7 +1439,7 @@ func TestSearchDashboards(t *testing.T) { }, Cells: [][]byte{ []byte("Dashboard 1"), - []byte(""), + []byte("f1"), []byte("[\"tag1\", \"tag2\"]"), }, }, @@ -1437,7 +1450,7 @@ func TestSearchDashboards(t *testing.T) { }, Cells: [][]byte{ []byte("Dashboard 2"), - []byte(""), + []byte("f1"), []byte(""), }, }, diff --git a/pkg/services/dashboards/service/search/search.go b/pkg/services/dashboards/service/search/search.go index 6165dfe0237..c0f3a5598f5 100644 --- a/pkg/services/dashboards/service/search/search.go +++ b/pkg/services/dashboards/service/search/search.go @@ -36,13 +36,13 @@ var ( } ) -func ParseResults(result *resource.ResourceSearchResponse, offset int64) (*v0alpha1.SearchResults, error) { +func ParseResults(result *resource.ResourceSearchResponse, offset int64) (v0alpha1.SearchResults, error) { if result == nil { - return nil, nil + return v0alpha1.SearchResults{}, nil } else if result.Error != nil { - return nil, fmt.Errorf("%d error searching: %s: %s", result.Error.Code, result.Error.Message, result.Error.Details) + return v0alpha1.SearchResults{}, fmt.Errorf("%d error searching: %s: %s", result.Error.Code, result.Error.Message, result.Error.Details) } else if result.Results == nil { - return nil, nil + return v0alpha1.SearchResults{}, nil } titleIDX := 0 @@ -66,7 +66,7 @@ func ParseResults(result *resource.ResourceSearchResponse, offset int64) (*v0alp } } - sr := &v0alpha1.SearchResults{ + sr := v0alpha1.SearchResults{ Offset: offset, TotalHits: result.TotalHits, QueryCost: result.QueryCost, @@ -80,7 +80,7 @@ func ParseResults(result *resource.ResourceSearchResponse, offset int64) (*v0alp if _, ok := excludedFields[col.Name]; !ok { val, err := resource.DecodeCell(col, colIndex, row.Cells[colIndex]) if err != nil { - return nil, err + return v0alpha1.SearchResults{}, err } // Some of the dashboard fields come in as int32, but we need to convert them to int64 or else fields.Set() will panic int32Val, ok := val.(int32)