diff --git a/pkg/apimachinery/validation/validation.go b/pkg/apimachinery/validation/validation.go new file mode 100644 index 00000000000..4778db398f3 --- /dev/null +++ b/pkg/apimachinery/validation/validation.go @@ -0,0 +1,105 @@ +package validation + +import ( + "regexp" + + "k8s.io/apimachinery/pkg/util/validation" +) + +const maxNameLength = 253 +const maxNamespaceLength = 40 +const minNamespaceLength = 3 +const maxGroupLength = 60 +const minGroupLength = 3 +const maxResourceLength = 40 +const minResourceLength = 3 + +const grafanaNameFmt = `^[a-zA-Z0-9:\-\_\.]*$` +const grafanaNameErrMsg string = "must consist of alphanumeric characters, '-', '_', ':' or '.'" + +const qnameCharFmt string = "[A-Za-z0-9]" +const qnameExtCharFmt string = "[-A-Za-z0-9_.]" +const qualifiedNameFmt string = "^(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt + "$" +const qualifiedNameErrMsg string = "must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character" + +const alphaCharFmt string = "[A-Za-z]" +const resourceCharFmt string = "[A-Za-z0-9]" // alpha numeric +const resourceFmt string = "^" + alphaCharFmt + resourceCharFmt + "*$" +const resourceErrMsg string = "must consist of alphanumeric characters" + +var ( + grafanaNameRegexp = regexp.MustCompile(grafanaNameFmt).MatchString + qualifiedNameRegexp = regexp.MustCompile(qualifiedNameFmt).MatchString + resourceRegexp = regexp.MustCompile(resourceFmt).MatchString +) + +// IsValidGrafanaName checks if the name is a valid to use for a k8s name +// Unlike normal k8s name rules, this allows the name to start with a digit +// This compromise means existing grafana UIDs are valid k8s names without migration +func IsValidGrafanaName(name string) []string { + s := len(name) + switch { + case s == 0: + return []string{"name may not be empty"} + case s > maxNameLength: + return []string{"name is too long"} + } + + if !grafanaNameRegexp(name) { + return []string{"name " + validation.RegexError(grafanaNameErrMsg, grafanaNameFmt, "MyName", "my.name", "abc-123")} + } + // In standard k8s, it must not start with a number + // however that would force us to update many many many existing resources + // so we will be slightly more lenient than standard k8s + return nil +} + +// If the value is not valid, a list of error strings is returned. +// Otherwise an empty list (or nil) is returned. +func IsValidNamespace(namespace string) []string { + s := len(namespace) + switch { + case s == 0: + return nil // empty is OK + case s > maxNamespaceLength: + return []string{"namespace is too long"} + case s < minNamespaceLength: + return []string{"namespace is too short"} + } + if !qualifiedNameRegexp(namespace) { + return []string{"namespace " + validation.RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "abc-123")} + } + return nil +} + +// If the value is not valid, a list of error strings is returned. +// Otherwise an empty list (or nil) is returned. +func IsValidGroup(group string) []string { + s := len(group) + switch { + case s > maxGroupLength: + return []string{"group is too long"} + case s < minGroupLength: + return []string{"group is too short"} + } + if !qualifiedNameRegexp(group) { + return []string{"group " + validation.RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "dashboards.grafana.app", "grafana-loki-datasource")} + } + return nil +} + +// If the value is not valid, a list of error strings is returned. +// Otherwise an empty list (or nil) is returned. +func IsValidateResource(resource string) []string { + s := len(resource) + switch { + case s > maxResourceLength: + return []string{"resource is too long"} + case s < minResourceLength: + return []string{"resource is too short"} + } + if !resourceRegexp(resource) { + return []string{"resource " + validation.RegexError(resourceErrMsg, resourceFmt, "dashboards", "folders")} + } + return nil +} diff --git a/pkg/apimachinery/validation/validation_test.go b/pkg/apimachinery/validation/validation_test.go new file mode 100644 index 00000000000..2e699097cdd --- /dev/null +++ b/pkg/apimachinery/validation/validation_test.go @@ -0,0 +1,230 @@ +package validation_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + k8sValidation "k8s.io/apimachinery/pkg/util/validation" + + "github.com/grafana/grafana/pkg/apimachinery/validation" +) + +func TestValidation(t *testing.T) { + // We are not using the out-of-the-box "isQualifiedName" because it allows slashes + rsp := k8sValidation.IsQualifiedName("hello/world") + require.Nil(t, rsp, "standard qualified name allows a slash") + + t.Run("name", func(t *testing.T) { + tests := []struct { + name string + input []string // variations that produce the same output + expect []string + }{{ + name: "empty", + input: []string{""}, + expect: []string{"name may not be empty"}, + }, { + name: "too long", + input: []string{strings.Repeat("0", 254)}, + expect: []string{"name is too long"}, + }, { + name: "ok", + input: []string{ + "hello", + strings.Repeat("0", 253), // very long starts with number + "hello-world", + "hello.world", + "hello_world", + "hello:world", + "123456", // starts with numbers + "aBCDEFG", // with capitals + }, + }, { + name: "bad input", + expect: []string{ + "name must consist of alphanumeric characters, '-', '_', ':' or '.' (e.g. 'MyName', or 'my.name', or 'abc-123', regex used for validation is '^[a-zA-Z0-9:\\-\\_\\.]*$')", + }, + input: []string{ + "hello world", + "hello!", + "hello~", + "hello ", + "hello*", + "hello+", + "hello=", + "hello%", + "hello/world", + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, input := range tt.input { + output := validation.IsValidGrafanaName(input) + require.Equal(t, tt.expect, output, "input: %s", input) + } + }) + } + }) + + t.Run("namespace", func(t *testing.T) { + tests := []struct { + name string + input []string // variations that produce the same output + expect []string + }{{ + name: "empty is OK", + input: []string{""}, + }, { + name: "too long", + input: []string{strings.Repeat("0", 41)}, + expect: []string{"namespace is too long"}, + }, { + name: "too short", + expect: []string{"namespace is too short"}, + input: []string{"a", "1", "aa"}, + }, { + name: "ok", + input: []string{ + "hello", + strings.Repeat("a", 40), // long... alpha + "hello-world", + "hello.world", + "hello_world", + "default", + "stacks-123456", // ends with a number + "org-3", // ends with a number + "1234", // just a numbers + "aaa", + }, + }, { + name: "bad input", + expect: []string{ + "namespace must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or 'abc-123', regex used for validation is '^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$')", + }, + input: []string{ + "_bad_input", // starts with non-alpha + "hello world", + "hello!", + "hello~", + "hello ", + "hello*", + "hello+", + "hello=", + "hello%", + "hello/world", + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, input := range tt.input { + output := validation.IsValidNamespace(input) + require.Equal(t, tt.expect, output, "input: %s", input) + } + }) + } + }) + + t.Run("group", func(t *testing.T) { + tests := []struct { + name string + input []string // variations that produce the same output + expect []string + }{{ + name: "too long", + expect: []string{"group is too long"}, + input: []string{strings.Repeat("0", 61)}, + }, { + name: "too short", + expect: []string{"group is too short"}, + input: []string{"a", "1", "aa"}, + }, { + name: "ok", + input: []string{ + "hello", + strings.Repeat("a", 60), // long... alpha + "dashboards.grafana.app", + "prometheus-datasource", + "1234", // just a numbers + "aaa", + }, + }, { + name: "bad input", + expect: []string{ + "group must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'dashboards.grafana.app', or 'grafana-loki-datasource', regex used for validation is '^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$')", + }, + input: []string{ + "_bad_input", // starts with non-alpha + "hello world", + "hello!", + "hello~", + "hello ", + "hello*", + "hello+", + "hello=", + "hello%", + "hello/world", + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, input := range tt.input { + output := validation.IsValidGroup(input) + require.Equal(t, tt.expect, output, "input: %s", input) + } + }) + } + }) + + t.Run("resource", func(t *testing.T) { + tests := []struct { + name string + input []string // variations that produce the same output + expect []string + }{{ + name: "too long", + expect: []string{"resource is too long"}, + input: []string{strings.Repeat("0", 41)}, + }, { + name: "too short", + expect: []string{"resource is too short"}, + input: []string{"a", "1", "aa"}, + }, { + name: "ok", + input: []string{ + "hello", + strings.Repeat("a", 40), // long... alpha + "dashboards", + "folders", + "folders123", + "aaa", + }, + }, { + name: "bad input", + expect: []string{ + "resource must consist of alphanumeric characters (e.g. 'dashboards', or 'folders', regex used for validation is '^[A-Za-z][A-Za-z0-9]*$')", + }, + input: []string{ + "_bad_input", + "hello world", + "hello-world", + "hello!", + "hello~", + "hello ", + "hello*", + "hello+", + "hello=", + "hello%", + "hello/world", + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, input := range tt.input { + output := validation.IsValidateResource(input) + require.Equal(t, tt.expect, output, "input: %s", input) + } + }) + } + }) +} diff --git a/pkg/registry/apis/iam/register.go b/pkg/registry/apis/iam/register.go index a92744700dc..c010698350d 100644 --- a/pkg/registry/apis/iam/register.go +++ b/pkg/registry/apis/iam/register.go @@ -152,6 +152,14 @@ func (b *IdentityAccessManagementAPIBuilder) AllowedV0Alpha1Resources() []string func (b *IdentityAccessManagementAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupInfo, opts builder.APIGroupOptions) error { storage := map[string]rest.Storage{} + // teams + users must have shorter names because they are often used as part of another name + opts.StorageOptsRegister(iamv0.TeamResourceInfo.GroupResource(), apistore.StorageOptions{ + MaximumNameLength: 80, + }) + opts.StorageOptsRegister(iamv0.UserResourceInfo.GroupResource(), apistore.StorageOptions{ + MaximumNameLength: 80, + }) + teamResource := iamv0.TeamResourceInfo teamLegacyStore := team.NewLegacyStore(b.store, b.legacyAccessClient, b.enableAuthnMutation) storage[teamResource.StoragePath()] = teamLegacyStore diff --git a/pkg/storage/unified/apistore/prepare.go b/pkg/storage/unified/apistore/prepare.go index a7f94a69706..3691cb29bbd 100644 --- a/pkg/storage/unified/apistore/prepare.go +++ b/pkg/storage/unified/apistore/prepare.go @@ -100,6 +100,9 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime if obj.GetFolder() != "" && !s.opts.EnableFolderSupport { return v, apierrors.NewBadRequest(fmt.Sprintf("folders are not supported for: %s", s.gr.String())) } + if s.opts.MaximumNameLength > 0 && len(obj.GetName()) > s.opts.MaximumNameLength { + return v, apierrors.NewBadRequest(fmt.Sprintf("name exceeds maximum length (%d)", s.opts.MaximumNameLength)) + } v.grantPermissions = obj.GetAnnotation(utils.AnnoKeyGrantPermissions) if v.grantPermissions != "" { diff --git a/pkg/storage/unified/apistore/prepare_test.go b/pkg/storage/unified/apistore/prepare_test.go index 3564e183e34..0b36f71117a 100644 --- a/pkg/storage/unified/apistore/prepare_test.go +++ b/pkg/storage/unified/apistore/prepare_test.go @@ -3,6 +3,7 @@ package apistore import ( "context" "math/rand/v2" + "strings" "testing" "time" @@ -35,6 +36,7 @@ func TestPrepareObjectForStorage(t *testing.T) { opts: StorageOptions{ EnableFolderSupport: true, LargeObjectSupport: nil, + MaximumNameLength: 100, }, } @@ -49,10 +51,18 @@ func TestPrepareObjectForStorage(t *testing.T) { }) t.Run("Error on missing name", func(t *testing.T) { - dashboard := dashv1.Dashboard{} - _, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject()) + dashboard := &dashv1.Dashboard{} + _, err := s.prepareObjectForStorage(ctx, dashboard) require.Error(t, err) - require.Contains(t, err.Error(), "missing name") + require.ErrorContains(t, err, "missing name") + }) + + t.Run("name is too long", func(t *testing.T) { + dashboard := &dashv1.Dashboard{} + dashboard.Name = strings.Repeat("a", 120) + _, err := s.prepareObjectForStorage(ctx, dashboard) + require.Error(t, err) + require.ErrorContains(t, err, "name exceeds maximum length") }) t.Run("Error on non-empty resource version", func(t *testing.T) { diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index 8b263fde25b..bf104949fa9 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -59,6 +59,9 @@ type StorageOptions struct { // Allow writing objects with metadata.annotations[grafana.app/folder] EnableFolderSupport bool + // Some resources should not allow the absolute maximum (254 characters) + MaximumNameLength int + // Add internalID label when missing RequireDeprecatedInternalID bool diff --git a/pkg/storage/unified/resource/keys.go b/pkg/storage/unified/resource/keys.go index 914a9429365..a9a5c4cabeb 100644 --- a/pkg/storage/unified/resource/keys.go +++ b/pkg/storage/unified/resource/keys.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/grafana/grafana/pkg/apimachinery/validation" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) @@ -17,17 +18,17 @@ 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 := validation.IsValidNamespace(key.Namespace); err != nil { + return NewBadRequestError(err[0]) } - if err := validateNamespace(key.Namespace); err != nil { - return NewBadRequestError(fmt.Sprintf("namespace '%s' is invalid: '%s'", key.Namespace, err)) + if err := validation.IsValidGroup(key.Group); err != nil { + return NewBadRequestError(err[0]) } - if err := validateGroup(key.Group); err != nil { - return NewBadRequestError(fmt.Sprintf("group '%s' is invalid: '%s'", key.Group, err)) + if err := validation.IsValidateResource(key.Resource); err != nil { + return NewBadRequestError(err[0]) } - if err := validateResource(key.Resource); err != nil { - return NewBadRequestError(fmt.Sprintf("resource '%s' is invalid: '%s'", key.Resource, err)) + if err := validation.IsValidGrafanaName(key.Name); err != nil { + return NewBadRequestError(err[0]) } return nil } diff --git a/pkg/storage/unified/resource/keys_test.go b/pkg/storage/unified/resource/keys_test.go index f6a894e3a1c..c53678cdcb6 100644 --- a/pkg/storage/unified/resource/keys_test.go +++ b/pkg/storage/unified/resource/keys_test.go @@ -81,7 +81,7 @@ func TestVerifyRequestKey(t *testing.T) { invalidNamespace := "(((((default" invalidName := " " // only spaces - namespaceTooLong := strings.Repeat("a", MaxNameLength+1) + namespaceTooLong := strings.Repeat("a", 61) nameTooLong := strings.Repeat("a", 300) tests := []struct { diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 451c731f486..575d1dd608b 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -24,6 +24,7 @@ import ( "github.com/grafana/dskit/ring" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/apimachinery/validation" secrets "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" "github.com/grafana/grafana/pkg/util/scheduler" @@ -523,8 +524,8 @@ func (s *server) newEvent(ctx context.Context, user claims.AuthInfo, key *resour return nil, NewBadRequestError( fmt.Sprintf("key/name do not match (key: %s, name: %s)", key.Name, obj.GetName())) } - if err := validateName(obj.GetName()); err != nil { - return nil, err + if errs := validation.IsValidGrafanaName(obj.GetName()); err != nil { + return nil, NewBadRequestError(errs[0]) } // For folder moves, we need to check permissions on both folders diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index 60f62b4df22..b2f1cd9d585 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -365,7 +365,7 @@ func TestSimpleServer(t *testing.T) { invalidQualifiedNames := []string{ "", // empty - strings.Repeat("1", MaxNameLength+1), // too long + strings.Repeat("1", 260), // too long " ", // only spaces "f8cc010c.ee72.4681;89d2+d46e1bd47d33", // invalid chars } diff --git a/pkg/storage/unified/resource/validation.go b/pkg/storage/unified/resource/validation.go deleted file mode 100644 index 1ba74263cd9..00000000000 --- a/pkg/storage/unified/resource/validation.go +++ /dev/null @@ -1,85 +0,0 @@ -package resource - -import ( - "fmt" - "regexp" - - "github.com/grafana/grafana/pkg/storage/unified/resourcepb" - "k8s.io/apimachinery/pkg/util/validation" -) - -const MaxNameLength = 253 -const MaxNamespaceLength = 40 -const MaxGroupLength = 60 -const MaxResourceLength = 40 - -var validNameCharPattern = `a-zA-Z0-9:\-\_\.` -var validNamePattern = regexp.MustCompile(`^[` + validNameCharPattern + `]*$`).MatchString - -func validateName(name string) *resourcepb.ErrorResult { - if len(name) == 0 { - return NewBadRequestError("name is too short") - } - if len(name) > MaxNameLength { - return NewBadRequestError("name is too long") - } - if !validNamePattern(name) { - return NewBadRequestError("name includes invalid characters") - } - // In standard k8s, it must not start with a number - // however that would force us to update many many many existing resources - // so we will be slightly more lenient than standard k8s - return nil -} - -func validateNamespace(value string) *resourcepb.ErrorResult { - if len(value) == 0 { - // empty namespace is allowed (means cluster-scoped) - return nil - } - - if len(value) > MaxNamespaceLength { - 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 -} - -func validateGroup(value string) *resourcepb.ErrorResult { - if len(value) == 0 { - return NewBadRequestError("value is too short") - } - - if len(value) > MaxGroupLength { - 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 -} - -func validateResource(value string) *resourcepb.ErrorResult { - if len(value) == 0 { - return NewBadRequestError("value is too short") - } - - if len(value) > MaxResourceLength { - 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 -} diff --git a/pkg/storage/unified/resource/validation_test.go b/pkg/storage/unified/resource/validation_test.go deleted file mode 100644 index bc0e4bd8bda..00000000000 --- a/pkg/storage/unified/resource/validation_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package resource - -import ( - "strings" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestNameValidation(t *testing.T) { - require.NotNil(t, validateName("")) // too short - require.NotNil(t, validateName(strings.Repeat("0", 254))) // too long (max 253) - - // OK - require.Nil(t, validateName("a")) - require.Nil(t, validateName("hello-world")) - require.Nil(t, validateName("hello.world")) - require.Nil(t, validateName("hello_world")) - require.Nil(t, validateName("hello:world")) - - // Bad characters - require.NotNil(t, validateName("hello world")) - require.NotNil(t, validateName("hello!")) - require.NotNil(t, validateName("hello~")) - require.NotNil(t, validateName("hello ")) - require.NotNil(t, validateName("hello*")) - require.NotNil(t, validateName("hello+")) - require.NotNil(t, validateName("hello=")) - require.NotNil(t, validateName("hello%")) -} diff --git a/pkg/storage/unified/testing/storage_backend.go b/pkg/storage/unified/testing/storage_backend.go index 1a13c310edd..712d1236475 100644 --- a/pkg/storage/unified/testing/storage_backend.go +++ b/pkg/storage/unified/testing/storage_backend.go @@ -1029,9 +1029,9 @@ func runTestIntegrationBlobSupport(t *testing.T, backend resource.StorageBackend t.Run("put and fetch blob", func(t *testing.T) { key := &resourcepb.ResourceKey{ Namespace: ns, - Group: "g", - Resource: "r", - Name: "n", + Group: "ggg", + Resource: "rrr", + Name: "nnn", } b1, err := server.PutBlob(ctx, &resourcepb.PutBlobRequest{ Resource: key,