Validation: Move validation into apimachinery package (#111736)

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
+}

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)
+				}
+			})
+		}
+	})
+}

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

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 != "" {

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{}
+		dashboard := &dashv1.Dashboard{}
-		_, err := s.prepareObjectForStorage(ctx, dashboard.DeepCopyObject())
+		_, 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) {

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
 

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 {
+	if err := validation.IsValidNamespace(key.Namespace); err != nil {
-		return NewBadRequestError(fmt.Sprintf("name '%s' is invalid: '%s'", key.Name, err))
+		return NewBadRequestError(err[0])
 	}
-	if err := validateNamespace(key.Namespace); err != nil {
+	if err := validation.IsValidGroup(key.Group); err != nil {
-		return NewBadRequestError(fmt.Sprintf("namespace '%s' is invalid: '%s'", key.Namespace, err))
+		return NewBadRequestError(err[0])
 	}
-	if err := validateGroup(key.Group); err != nil {
+	if err := validation.IsValidateResource(key.Resource); err != nil {
-		return NewBadRequestError(fmt.Sprintf("group '%s' is invalid: '%s'", key.Group, err))
+		return NewBadRequestError(err[0])
 	}
-	if err := validateResource(key.Resource); err != nil {
+	if err := validation.IsValidGrafanaName(key.Name); err != nil {
-		return NewBadRequestError(fmt.Sprintf("resource '%s' is invalid: '%s'", key.Resource, err))
+		return NewBadRequestError(err[0])
 	}
 	return nil
 }

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 {

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 {
+	if errs := validation.IsValidGrafanaName(obj.GetName()); err != nil {
-		return nil, err
+		return nil, NewBadRequestError(errs[0])
 	}
 
 	// For folder moves, we need to check permissions on both folders

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
 		}

pkg/storage/unified/resource/validation.go

@@ -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
-}

pkg/storage/unified/resource/validation_test.go

@@ -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%"))
-}

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",
+			Group:     "ggg",
-			Resource:  "r",
+			Resource:  "rrr",
-			Name:      "n",
+			Name:      "nnn",
 		}
 		b1, err := server.PutBlob(ctx, &resourcepb.PutBlobRequest{
 			Resource:    key,