From b1157dbd7a0ceb4a2c907eb63dbc3dd93afbb882 Mon Sep 17 00:00:00 2001 From: Leonor Oliveira <9090754+leonorfmartins@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:19:53 +0100 Subject: [PATCH] Validate if we are creating a folder more than 5 levels deep (#95579) * Add getter to FolderAPIBuilder so that we can access it in admission * Remove deprecated return * Fix test * Update pkg/registry/apis/folders/register_test.go Co-authored-by: maicon * Fix maxNested folder test * Remove log --------- Co-authored-by: maicon --- pkg/registry/apis/folders/mocks.go | 69 ++++++++++++++++++++++ pkg/registry/apis/folders/register.go | 32 +++++----- pkg/registry/apis/folders/register_test.go | 53 +++++++++++++++-- 3 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 pkg/registry/apis/folders/mocks.go diff --git a/pkg/registry/apis/folders/mocks.go b/pkg/registry/apis/folders/mocks.go new file mode 100644 index 00000000000..65f322ce251 --- /dev/null +++ b/pkg/registry/apis/folders/mocks.go @@ -0,0 +1,69 @@ +package folders + +import ( + "context" + + "github.com/stretchr/testify/mock" + "k8s.io/apimachinery/pkg/apis/meta/internalversion" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/registry/rest" +) + +type storageMock struct { + *mock.Mock + rest.Storage +} + +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) +} + +func (m storageMock) ConvertToTable(ctx context.Context, obj runtime.Object, tableOptions runtime.Object) (*metav1.Table, error) { + args := m.Called(ctx, obj, tableOptions) + return args.Get(0).(*metav1.Table), args.Error(1) +} + +func (m storageMock) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + args := m.Called(ctx, obj, createValidation, options) + return args.Get(0).(runtime.Object), args.Error(1) +} + +func (m storageMock) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { + args := m.Called(ctx, name, deleteValidation, options) + return args.Get(0).(runtime.Object), args.Bool(1), args.Error(2) +} + +func (m storageMock) DeleteCollection(ctx context.Context, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions, listOptions *internalversion.ListOptions) (runtime.Object, error) { + args := m.Called(ctx, deleteValidation, options, listOptions) + return args.Get(0).(runtime.Object), args.Error(1) +} + +func (m storageMock) GetSingularName() string { + args := m.Called() + return args.String(0) +} + +func (m storageMock) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) { + args := m.Called(ctx, options) + return args.Get(0).(runtime.Object), args.Error(1) +} + +func (m storageMock) NamespaceScoped() bool { + args := m.Called() + return args.Bool(0) +} + +func (m storageMock) NewList() runtime.Object { + args := m.Called() + return args.Get(0).(runtime.Object) +} + +func (m storageMock) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) { + args := m.Called(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) + if name == "object-fail" { + return nil, false, args.Error(2) + } + return args.Get(0).(runtime.Object), args.Bool(1), args.Error(2) +} diff --git a/pkg/registry/apis/folders/register.go b/pkg/registry/apis/folders/register.go index 80444b8d3fb..e065b9c9c65 100644 --- a/pkg/registry/apis/folders/register.go +++ b/pkg/registry/apis/folders/register.go @@ -4,6 +4,7 @@ import ( "context" "errors" + grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/prometheus/client_golang/prometheus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -36,20 +37,13 @@ var resourceInfo = v0alpha1.FolderResourceInfo var errNoUser = errors.New("valid user is required") var errNoResource = errors.New("resource name is required") -var folderValidationRules = struct { - maxDepth int - invalidNames []string -}{ - maxDepth: 4, - invalidNames: []string{"general"}, -} - // This is used just so wire has something unique to return type FolderAPIBuilder struct { gv schema.GroupVersion features featuremgmt.FeatureToggles namespacer request.NamespaceMapper folderSvc folder.Service + storage grafanarest.Storage accessControl accesscontrol.AccessControl } @@ -138,6 +132,7 @@ func (b *FolderAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.API } apiGroupInfo.VersionedResourcesStorageMap[v0alpha1.VERSION] = storage + b.storage = storage[resourceInfo.StoragePath()].(grafanarest.Storage) return nil } @@ -225,8 +220,15 @@ func authorizerFunc(ctx context.Context, attr authorizer.Attributes) (*authorize return &authorizerParams{evaluator: eval, user: user}, nil } +var folderValidationRules = struct { + maxDepth int + invalidNames []string +}{ + maxDepth: 5, + invalidNames: []string{"general"}, +} + func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error { - // name cannot be called "general" id := a.GetName() for _, invalidName := range folderValidationRules.invalidNames { if id == invalidName { @@ -236,18 +238,20 @@ func (b *FolderAPIBuilder) Validate(ctx context.Context, a admission.Attributes, obj := a.GetObject() - for i := 0; i <= folderValidationRules.maxDepth+1; i++ { + for i := 1; i <= folderValidationRules.maxDepth; i++ { parent := getParent(obj) if parent == "" { break } - if i == folderValidationRules.maxDepth+1 { + if i == folderValidationRules.maxDepth { return folder.ErrMaximumDepthReached } - // TODO: investigate the best way to get a folder by name, considering validate is called before the storage layer - // parent, err := getFolderByNameSomehow() - // obj = parent + parentObj, err := b.storage.Get(ctx, parent, &metav1.GetOptions{}) + if err != nil { + return err + } + obj = parentObj } return nil } diff --git a/pkg/registry/apis/folders/register_test.go b/pkg/registry/apis/folders/register_test.go index dcd9e96aef3..050e9a638e1 100644 --- a/pkg/registry/apis/folders/register_test.go +++ b/pkg/registry/apis/folders/register_test.go @@ -7,12 +7,15 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apis/folder/v0alpha1" + grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/authz/zanzana" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/foldertest" "github.com/grafana/grafana/pkg/services/user" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apiserver/pkg/admission" @@ -124,12 +127,13 @@ func TestFolderAPIBuilder_Validate(t *testing.T) { name string } tests := []struct { - name string - input input - err error + name string + input input + setupFn func(*mock.Mock) + err error }{ { - name: "should return error when nameis invalid", + name: "should return error when name is invalid", input: input{ obj: &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -140,17 +144,58 @@ func TestFolderAPIBuilder_Validate(t *testing.T) { }, err: dashboards.ErrFolderInvalidUID, }, + { + name: "should return no error if every validation passes", + input: input{ + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "meta": map[string]interface{}{"name": "valid-name"}, + }, + }, + name: "valid-name", + }, + }, + { + name: "should return error when creating a nested folder higher than max depth", + input: input{ + obj: &unstructured.Unstructured{ + Object: map[string]any{ + "metadata": map[string]any{"name": "valid-name", "annotations": map[string]any{"grafana.app/folder": "valid-name"}}, + }, + }, + name: "valid-name", + }, + setupFn: func(m *mock.Mock) { + m.On("Get", mock.Anything, "valid-name", mock.Anything).Return( + &unstructured.Unstructured{ + Object: map[string]any{ + "metadata": map[string]any{"name": "valid-name", "annotations": map[string]any{"grafana.app/folder": "valid-name"}}, + }, + }, nil) + }, + err: folder.ErrMaximumDepthReached, + }, } + s := (grafanarest.Storage)(nil) + m := &mock.Mock{} + us := storageMock{m, s} + 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()), } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.setupFn != nil { + tt.setupFn(m) + } + err := b.Validate(context.Background(), admission.NewAttributesRecord( tt.input.obj, nil,