From b3ed600bca1272a503e344a4c3d09a8ae7bac6f1 Mon Sep 17 00:00:00 2001 From: maicon Date: Fri, 19 Sep 2025 15:03:23 -0300 Subject: [PATCH] Revert "Unistore: Add validation for resource names" (#111408) Revert "Unistore: Add validation for resource names (#110990)" This reverts commit b63e3fd3aebb73fa1d0b6c1f97334660e18a2a25. --- .../storage/testing/watcher_tests.go | 8 +- pkg/storage/unified/resource/bulk.go | 12 -- pkg/storage/unified/resource/keys.go | 12 -- pkg/storage/unified/resource/keys_test.go | 115 ---------- pkg/storage/unified/resource/server.go | 4 - pkg/storage/unified/resource/server_test.go | 197 ------------------ pkg/storage/unified/resource/validation.go | 20 -- 7 files changed, 4 insertions(+), 364 deletions(-) diff --git a/pkg/apiserver/storage/testing/watcher_tests.go b/pkg/apiserver/storage/testing/watcher_tests.go index 371a3d612f9..89ea37d4b95 100644 --- a/pkg/apiserver/storage/testing/watcher_tests.go +++ b/pkg/apiserver/storage/testing/watcher_tests.go @@ -684,7 +684,7 @@ func RunTestClusterScopedWatch(ctx context.Context, t *testing.T, store storage. currentObjs := map[string]*example.Pod{} for _, watchTest := range tt.watchTests { out := &example.Pod{} - key := "pods/ns-1/" + watchTest.obj.Name + key := "pods/" + watchTest.obj.Name err := store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { obj := watchTest.obj.DeepCopy() @@ -1607,15 +1607,15 @@ func clusterScopedNodeNameAttrFunc(obj runtime.Object) (labels.Set, fields.Set, } func basePod(podName string) *example.Pod { - return baseNamespacedPod(podName, "ns-1") + return baseNamespacedPod(podName, "") } func basePodUpdated(podName string) *example.Pod { - return baseNamespacedPodUpdated(podName, "ns-1") + return baseNamespacedPodUpdated(podName, "") } func basePodAssigned(podName, nodeName string) *example.Pod { - return baseNamespacedPodAssigned(podName, "ns-1", nodeName) + return baseNamespacedPodAssigned(podName, "", nodeName) } func baseNamespacedPod(podName, namespace string) *example.Pod { diff --git a/pkg/storage/unified/resource/bulk.go b/pkg/storage/unified/resource/bulk.go index a0720efa6de..90298c0da66 100644 --- a/pkg/storage/unified/resource/bulk.go +++ b/pkg/storage/unified/resource/bulk.go @@ -170,18 +170,6 @@ func (s *server) BulkProcess(stream resourcepb.BulkStore_BulkProcessServer) erro }) } - // Verify all request keys are valid - for _, k := range settings.Collection { - if r := verifyRequestKey(k); r != nil { - return sendAndClose(&resourcepb.BulkResponse{ - Error: &resourcepb.ErrorResult{ - Message: fmt.Sprintf("invalid request key: %s", r.Message), - Code: http.StatusBadRequest, - }, - }) - } - } - if settings.RebuildCollection { for _, k := range settings.Collection { // Can we delete the whole collection diff --git a/pkg/storage/unified/resource/keys.go b/pkg/storage/unified/resource/keys.go index 204246f3497..0af18d6c048 100644 --- a/pkg/storage/unified/resource/keys.go +++ b/pkg/storage/unified/resource/keys.go @@ -17,18 +17,6 @@ func verifyRequestKey(key *resourcepb.ResourceKey) *resourcepb.ErrorResult { if key.Resource == "" { return NewBadRequestError("request key is missing resource") } - if err := validateName(key.Name); err != nil { - return NewBadRequestError(fmt.Sprintf("name '%s' is invalid: '%s'", key.Name, err)) - } - if err := validateQualifiedName(key.Namespace); err != nil { - return NewBadRequestError(fmt.Sprintf("namespace '%s' is invalid: '%s'", key.Namespace, err)) - } - if err := validateQualifiedName(key.Group); err != nil { - return NewBadRequestError(fmt.Sprintf("group '%s' is invalid: '%s'", key.Group, err)) - } - if err := validateQualifiedName(key.Resource); err != nil { - return NewBadRequestError(fmt.Sprintf("resource '%s' is invalid: '%s'", key.Resource, err)) - } return nil } diff --git a/pkg/storage/unified/resource/keys_test.go b/pkg/storage/unified/resource/keys_test.go index f86f1224313..d14c03fcc95 100644 --- a/pkg/storage/unified/resource/keys_test.go +++ b/pkg/storage/unified/resource/keys_test.go @@ -1,8 +1,6 @@ package resource import ( - "net/http" - "strings" "testing" "github.com/stretchr/testify/require" @@ -68,116 +66,3 @@ func TestSearchIDKeys(t *testing.T) { } } } - -func TestVerifyRequestKey(t *testing.T) { - validGroup := "group.grafana.app" - validResource := "resource" - validNamespace := "default" - validName := "fdgsv37qslr0ga" - validLegacyUID := "f8cc010c-ee72-4681-89d2-d46e1bd47d33" - - invalidGroup := "group.~~~~~grafana.app" - invalidResource := "##resource" - invalidNamespace := "(((((default" - invalidName := " " // only spaces - - namespaceTooLong := strings.Repeat("a", MaxQualifiedNameLength+1) - nameTooLong := strings.Repeat("a", 300) - - tests := []struct { - name string - input *resourcepb.ResourceKey - expectedCode int32 - }{ - { - name: "no error when all fields are set and valid", - input: &resourcepb.ResourceKey{ - Namespace: validNamespace, - Group: validGroup, - Resource: validResource, - Name: validName, - }, - }, - { - name: "invalid namespace returns error", - input: &resourcepb.ResourceKey{ - Namespace: invalidNamespace, - Group: validGroup, - Resource: validResource, - Name: validName, - }, - expectedCode: http.StatusBadRequest, - }, - { - name: "invalid group returns error", - input: &resourcepb.ResourceKey{ - Namespace: validNamespace, - Group: invalidGroup, - Resource: validResource, - Name: validName, - }, - expectedCode: http.StatusBadRequest, - }, - { - name: "invalid resource returns error", - input: &resourcepb.ResourceKey{ - Namespace: validNamespace, - Group: validGroup, - Resource: invalidResource, - Name: validName, - }, - expectedCode: http.StatusBadRequest, - }, - { - name: "invalid name returns error", - input: &resourcepb.ResourceKey{ - Namespace: validNamespace, - Group: validGroup, - Resource: validResource, - Name: invalidName, - }, - expectedCode: http.StatusBadRequest, - }, - { - name: "valid legacy UID returns no error", - input: &resourcepb.ResourceKey{ - Namespace: validNamespace, - Group: validGroup, - Resource: validResource, - Name: validLegacyUID, - }, - }, - { - name: "namespace too long returns error", - input: &resourcepb.ResourceKey{ - Namespace: namespaceTooLong, - Group: validGroup, - Resource: validResource, - Name: validName, - }, - expectedCode: http.StatusBadRequest, - }, - { - name: "name too long returns error", - input: &resourcepb.ResourceKey{ - Namespace: namespaceTooLong, - Group: validGroup, - Resource: validResource, - Name: nameTooLong, - }, - expectedCode: http.StatusBadRequest, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - err := verifyRequestKey(test.input) - if test.expectedCode == 0 { - require.Nil(t, err) - return - } - - require.Equal(t, test.expectedCode, err.Code) - }) - } -} diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 55fe947fa56..35987a418b8 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -626,10 +626,6 @@ func (s *server) Create(ctx context.Context, req *resourcepb.CreateRequest) (*re ctx, span := s.tracer.Start(ctx, "storage_server.Create") defer span.End() - if r := verifyRequestKey(req.Key); r != nil { - return nil, fmt.Errorf("invalid request key: %s", r.Message) - } - rsp := &resourcepb.CreateResponse{} user, ok := claims.AuthInfoFrom(ctx) if !ok || user == nil { diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index 150ee328af1..98bc928da63 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -7,7 +7,6 @@ import ( "log/slog" "net/http" "os" - "strings" "sync" "testing" "time" @@ -196,202 +195,6 @@ func TestSimpleServer(t *testing.T) { require.Len(t, all.Items, 0) // empty }) - t.Run("playlist FAIL CRUD paths due to invalid key", func(t *testing.T) { - raw := []byte(`{ - "apiVersion": "playlist.grafana.app/v0alpha1", - "kind": "Playlist", - "metadata": { - "name": "fdgsv37#qslr0ga", - "uid": "xyz", - "namespace": "default", - "annotations": { - "grafana.app/repoName": "elsewhere", - "grafana.app/repoPath": "path/to/item", - "grafana.app/repoTimestamp": "2024-02-02T00:00:00Z" - } - }, - "spec": { - "title": "hello", - "interval": "5m", - "items": [ - { - "type": "dashboard_by_uid", - "value": "vmie2cmWz" - } - ] - } - }`) - - // invalid group - key := &resourcepb.ResourceKey{ - Group: "playlist.grafana.app###", - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "fdgsv37qslr0ga", - } - - created, err := server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - require.Error(t, err) - require.Nil(t, created) - - // invalid resource - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr###", // can be anything :( - Namespace: "default", - Name: "fdgsv37qslr0ga", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - require.Error(t, err) - require.Nil(t, created) - - // invalid namespace - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: "default###", - Name: "fdgsv37qslr0ga", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - require.Error(t, err) - require.Nil(t, created) - - // invalid name - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "fdgsv37qslr0g###", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - require.Error(t, err) - require.Nil(t, created) - - // legacy name - valid - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "2c7e5361-7360-4d2a-ae45-5e79bba458d6", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.NoError(t, err) - require.NotNil(t, created) - - // legacy name - also valid - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "IvIsO_YGz", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.NoError(t, err) - require.NotNil(t, created) - - // legacy name - also valid - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "_IvIsOYGz", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.NoError(t, err) - require.NotNil(t, created) - - invalidQualifiedNames := []string{ - "", // empty - strings.Repeat("1", MaxQualifiedNameLength+1), // too long - " ", // only spaces - "f8cc010c.ee72.4681;89d2+d46e1bd47d33", // invalid chars - } - - // group - for _, invalidGroup := range invalidQualifiedNames { - key = &resourcepb.ResourceKey{ - Group: invalidGroup, - Resource: "rrrr", // can be anything :( - Namespace: "default", - Name: "_IvIsOYGz", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.Error(t, err) - require.Nil(t, created) - } - - // resource - for _, invalidResource := range invalidQualifiedNames { - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: invalidResource, - Namespace: "default", - Name: "_IvIsOYGz", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.Error(t, err) - require.Nil(t, created) - } - - // namespace - for _, invalidNamespace := range invalidQualifiedNames { - key = &resourcepb.ResourceKey{ - Group: "playlist.grafana.app", - Resource: "rrrr", // can be anything :( - Namespace: invalidNamespace, - Name: "_IvIsOYGz", - } - - created, err = server.Create(ctx, &resourcepb.CreateRequest{ - Value: raw, - Key: key, - }) - - require.Error(t, err) - require.Nil(t, created) - } - }) - t.Run("playlist update optimistic concurrency check", func(t *testing.T) { raw := []byte(`{ "apiVersion": "playlist.grafana.app/v0alpha1", diff --git a/pkg/storage/unified/resource/validation.go b/pkg/storage/unified/resource/validation.go index c3d3ee5bd45..2c1b5cf3740 100644 --- a/pkg/storage/unified/resource/validation.go +++ b/pkg/storage/unified/resource/validation.go @@ -1,15 +1,11 @@ package resource import ( - "fmt" "regexp" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" - "k8s.io/apimachinery/pkg/util/validation" ) -const MaxQualifiedNameLength = 40 - var validNameCharPattern = `a-zA-Z0-9:\-\_\.` var validNamePattern = regexp.MustCompile(`^[` + validNameCharPattern + `]*$`).MatchString @@ -28,19 +24,3 @@ func validateName(name string) *resourcepb.ErrorResult { // so we will be slightly more lenient than standard k8s return nil } - -func validateQualifiedName(value string) *resourcepb.ErrorResult { - if len(value) == 0 { - return NewBadRequestError("value is too short") - } - if len(value) > MaxQualifiedNameLength { - return NewBadRequestError("value is too long") - } - - err := validation.IsQualifiedName(value) - if len(err) > 0 { - return NewBadRequestError(fmt.Sprintf("name is not a valid qualified name: %+v", err)) - } - - return nil -}