From f710573b37e6764ae879f4bbcc145dee485b0ade Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Thu, 12 Dec 2024 11:53:01 +0100 Subject: [PATCH] Use searcher GetStats to count for folders and dashboards under a folder to be deleted (#97708) * get stats from index * fix logging * fix logging * actually counting * Use searcher to check for resources referencing a folder * Add tests when deleting a folder * Lint * merge/fix test * will delegate to SQL * get stats from index * fix logging * actually counting * merge/fix test * will delegate to SQL * Uncomment test * Add tests for checking stats before deleting a folder * Change base branch to main * Lint * [REVIEW] remove logs * Check for type assertion success --------- Co-authored-by: Ryan McKinley --- pkg/registry/apis/folders/mocks.go | 12 ++++ pkg/registry/apis/folders/register.go | 63 +++++++++++++--- pkg/registry/apis/folders/register_test.go | 83 +++++++++++++++++++++- 3 files changed, 145 insertions(+), 13 deletions(-) diff --git a/pkg/registry/apis/folders/mocks.go b/pkg/registry/apis/folders/mocks.go index 65f322ce251..ee722574aab 100644 --- a/pkg/registry/apis/folders/mocks.go +++ b/pkg/registry/apis/folders/mocks.go @@ -3,7 +3,9 @@ package folders import ( "context" + "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/stretchr/testify/mock" + "google.golang.org/grpc" "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -15,6 +17,11 @@ type storageMock struct { rest.Storage } +type searcherMock struct { + *mock.Mock + resource.ResourceIndexClient +} + func (m storageMock) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { args := m.Called(ctx, name, options) return args.Get(0).(runtime.Object), args.Error(1) @@ -67,3 +74,8 @@ func (m storageMock) Update(ctx context.Context, name string, objInfo rest.Updat } return args.Get(0).(runtime.Object), args.Bool(1), args.Error(2) } + +func (s searcherMock) GetStats(ctx context.Context, req *resource.ResourceStatsRequest, opts ...grpc.CallOption) (*resource.ResourceStatsResponse, error) { + args := s.Called(ctx, req) + return args.Get(0).(*resource.ResourceStatsResponse), args.Error(1) +} diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index 2e67f7d4195..a79e2c8f32c 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -17,11 +17,13 @@ import ( common "k8s.io/kube-openapi/pkg/common" "k8s.io/kube-openapi/pkg/spec3" + grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" + "github.com/grafana/grafana/pkg/storage/unified/resource" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic" - grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/apiserver/builder" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" @@ -29,7 +31,6 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/storage/unified/resource" ) var _ builder.APIGroupBuilder = (*FolderAPIBuilder)(nil) @@ -127,9 +128,7 @@ func (b *FolderAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.API storage[resourceInfo.StoragePath()] = legacyStore storage[resourceInfo.StoragePath("parents")] = &subParentsREST{b.folderSvc} storage[resourceInfo.StoragePath("access")] = &subAccessREST{b.folderSvc} - storage[resourceInfo.StoragePath("count")] = &subCountREST{ - searcher: b.searcher, - } + storage[resourceInfo.StoragePath("count")] = &subCountREST{searcher: b.searcher} // enable dual writer if optsGetter != nil && dualWriteBuilder != nil { @@ -241,12 +240,6 @@ var folderValidationRules = struct { func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error { id := a.GetName() - for _, invalidName := range folderValidationRules.invalidNames { - if id == invalidName { - return dashboards.ErrFolderInvalidUID - } - } - obj := a.GetObject() if obj == nil || a.GetOperation() == admission.Connect { return nil // This is normal for sub-resource @@ -256,7 +249,55 @@ func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes, if !ok { return fmt.Errorf("obj is not v0alpha1.Folder") } + verb := a.GetOperation() + switch verb { + case admission.Create: + return b.validateOnCreate(ctx, id, obj) + case admission.Delete: + return b.validateOnDelete(ctx, f) + case admission.Update: + return nil + case admission.Connect: + return nil + } + return nil +} + +func (b *FolderAPIBuilder) validateOnDelete(ctx context.Context, f *v0alpha1.Folder) error { + resp, err := b.searcher.GetStats(ctx, &resource.ResourceStatsRequest{Namespace: f.Namespace, Folder: f.Name}) + if err != nil { + return err + } + + if resp != nil && resp.Error != nil { + return fmt.Errorf("could not verify if folder is empty: %v", resp.Error) + } + + if resp.Stats == nil { + return fmt.Errorf("could not verify if folder is empty: %v", resp.Error) + } + + for _, v := range resp.Stats { + if v.Count > 0 { + return folder.ErrFolderNotEmpty + } + } + + return nil +} + +func (b *FolderAPIBuilder) validateOnCreate(ctx context.Context, id string, obj runtime.Object) error { + for _, invalidName := range folderValidationRules.invalidNames { + if id == invalidName { + return dashboards.ErrFolderInvalidUID + } + } + + f, ok := obj.(*v0alpha1.Folder) + if !ok { + return fmt.Errorf("obj is not v0alpha1.Folder") + } if f.Spec.Title == "" { return dashboards.ErrFolderTitleEmpty } diff --git a/pkg/registry/apis/folders/register_test.go b/pkg/registry/apis/folders/register_test.go index b3ff66b2393..1409f3e37ab 100644 --- a/pkg/registry/apis/folders/register_test.go +++ b/pkg/registry/apis/folders/register_test.go @@ -15,8 +15,10 @@ import ( "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/foldertest" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" ) @@ -210,7 +212,7 @@ func TestFolderAPIBuilder_getAuthorizerFunc(t *testing.T) { } } -func TestFolderAPIBuilder_Validate(t *testing.T) { +func TestFolderAPIBuilder_Validate_Create(t *testing.T) { type input struct { obj *v0alpha1.Folder annotations map[string]string @@ -316,7 +318,7 @@ func TestFolderAPIBuilder_Validate(t *testing.T) { tt.input.name, v0alpha1.SchemeGroupVersion.WithResource("folders"), "", - "create", + "CREATE", nil, true, &user.SignedInUser{}, @@ -331,3 +333,80 @@ func TestFolderAPIBuilder_Validate(t *testing.T) { }) } } + +func TestFolderAPIBuilder_Validate_Delete(t *testing.T) { + tests := []struct { + name string + statsResponse *resource.ResourceStatsResponse_Stats + wantErr bool + }{ + { + name: "should allow deletion when folder is empty", + statsResponse: &resource.ResourceStatsResponse_Stats{Count: 0}, + }, + { + name: "should return folder not empty when the folder is not empty", + statsResponse: &resource.ResourceStatsResponse_Stats{Count: 2}, + wantErr: true, + }, + } + + s := (grafanarest.Storage)(nil) + m := &mock.Mock{} + us := storageMock{m, s} + sm := searcherMock{Mock: m} + + obj := &v0alpha1.Folder{ + Spec: v0alpha1.Spec{ + Title: "foo", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "stacks-123", + Name: "valid-name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var setupFn = func(m *mock.Mock, stats *resource.ResourceStatsResponse_Stats) { + m.On("GetStats", mock.Anything, &resource.ResourceStatsRequest{Namespace: obj.Namespace, Folder: obj.Name}).Return( + &resource.ResourceStatsResponse{Stats: []*resource.ResourceStatsResponse_Stats{stats}}, + nil, + ).Once() + } + + setupFn(m, tt.statsResponse) + + b := &FolderAPIBuilder{ + gv: resourceInfo.GroupVersion(), + features: nil, + namespacer: func(_ int64) string { return "123" }, + folderSvc: foldertest.NewFakeService(), + storage: us, + accessControl: acimpl.ProvideAccessControl(featuremgmt.WithFeatures("nestedFolders"), zanzana.NewNoopClient()), + searcher: sm, + } + + err := b.Validate(context.Background(), admission.NewAttributesRecord( + obj, + nil, + v0alpha1.SchemeGroupVersion.WithKind("folder"), + obj.Namespace, + obj.Name, + v0alpha1.SchemeGroupVersion.WithResource("folders"), + "", + "DELETE", + nil, + true, + &user.SignedInUser{}, + ), + nil) + + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +}