diff --git a/pkg/apimachinery/validation/validation.go b/pkg/apimachinery/validation/validation.go index 777eb59fa46..6cdbb6b7e78 100644 --- a/pkg/apimachinery/validation/validation.go +++ b/pkg/apimachinery/validation/validation.go @@ -90,7 +90,7 @@ func IsValidGroup(group string) []string { // 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 { +func IsValidResource(resource string) []string { s := len(resource) switch { case s > maxResourceLength: diff --git a/pkg/apimachinery/validation/validation_test.go b/pkg/apimachinery/validation/validation_test.go index 3bb5fa61ad9..380f69fd362 100644 --- a/pkg/apimachinery/validation/validation_test.go +++ b/pkg/apimachinery/validation/validation_test.go @@ -222,7 +222,7 @@ func TestValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { for _, input := range tt.input { - output := validation.IsValidateResource(input) + output := validation.IsValidResource(input) require.Equal(t, tt.expect, output, "input: %s", input) } }) diff --git a/pkg/storage/unified/resource/datastore.go b/pkg/storage/unified/resource/datastore.go index e45e1b74101..d5fc0cdf915 100644 --- a/pkg/storage/unified/resource/datastore.go +++ b/pkg/storage/unified/resource/datastore.go @@ -2,15 +2,16 @@ package resource import ( "context" + "errors" "fmt" "io" "iter" "math" - "regexp" "strconv" "strings" "time" + "github.com/grafana/grafana/pkg/apimachinery/validation" gocache "github.com/patrickmn/go-cache" ) @@ -54,12 +55,6 @@ type GroupResource struct { Resource string } -var ( - // validNameRegex validates that a name contains only lowercase alphanumeric characters, '-' or '.' - // and starts and ends with an alphanumeric character - validNameRegex = regexp.MustCompile(`^[a-z0-9]([a-z0-9.-]*[a-z0-9])?$`) -) - func (k DataKey) String() string { return fmt.Sprintf("%s/%s/%s/%s/%d~%s~%s", k.Group, k.Resource, k.Namespace, k.Name, k.ResourceVersion, k.Action, k.Folder) } @@ -69,42 +64,35 @@ func (k DataKey) Equals(other DataKey) bool { } func (k DataKey) Validate() error { - if k.Group == "" { - return fmt.Errorf("group is required") - } - if k.Resource == "" { - return fmt.Errorf("resource is required") - } if k.Namespace == "" { - return fmt.Errorf("namespace is required") - } - if k.Name == "" { - return fmt.Errorf("name is required") + return NewValidationError("namespace", k.Namespace, ErrNamespaceRequired) } if k.ResourceVersion <= 0 { - return fmt.Errorf("resource version must be positive") + return NewValidationError("resourceVersion", fmt.Sprintf("%d", k.ResourceVersion), ErrResourceVersionInvalid) } if k.Action == "" { - return fmt.Errorf("action is required") + return NewValidationError("action", string(k.Action), ErrActionRequired) } // Validate naming conventions for all required fields - if !validNameRegex.MatchString(k.Namespace) { - return fmt.Errorf("namespace '%s' is invalid", k.Namespace) + if err := validation.IsValidNamespace(k.Namespace); err != nil { + return NewValidationError("namespace", k.Namespace, err[0]) } - if !validNameRegex.MatchString(k.Group) { - return fmt.Errorf("group '%s' is invalid", k.Group) + if err := validation.IsValidGroup(k.Group); err != nil { + return NewValidationError("group", k.Group, err[0]) } - if !validNameRegex.MatchString(k.Resource) { - return fmt.Errorf("resource '%s' is invalid", k.Resource) + if err := validation.IsValidResource(k.Resource); err != nil { + return NewValidationError("resource", k.Resource, err[0]) } - if !validNameRegex.MatchString(k.Name) { - return fmt.Errorf("name '%s' is invalid", k.Name) + if err := validation.IsValidGrafanaName(k.Name); err != nil { + return NewValidationError("name", k.Name, err[0]) } // Validate folder field if provided (optional field) - if k.Folder != "" && !validNameRegex.MatchString(k.Folder) { - return fmt.Errorf("folder '%s' is invalid", k.Folder) + if k.Folder != "" { + if err := validation.IsValidGrafanaName(k.Folder); err != nil { + return NewValidationError("folder", k.Folder, err[0]) + } } // Validate action is one of the valid values @@ -124,27 +112,21 @@ type ListRequestKey struct { } func (k ListRequestKey) Validate() error { - if k.Group == "" { - return fmt.Errorf("group is required") - } - if k.Resource == "" { - return fmt.Errorf("resource is required") - } if k.Namespace == "" && k.Name != "" { - return fmt.Errorf("name must be empty when namespace is empty") + return errors.New(ErrNameMustBeEmptyWhenNamespaceEmpty) } - if k.Namespace != "" && !validNameRegex.MatchString(k.Namespace) { - return fmt.Errorf("namespace '%s' is invalid", k.Namespace) + if k.Namespace != "" { + if err := validation.IsValidNamespace(k.Namespace); err != nil { + return NewValidationError("namespace", k.Namespace, err[0]) + } } - if !validNameRegex.MatchString(k.Group) { - return fmt.Errorf("group '%s' is invalid", k.Group) + if err := validation.IsValidGroup(k.Group); err != nil { + return NewValidationError("group", k.Group, err[0]) } - if !validNameRegex.MatchString(k.Resource) { - return fmt.Errorf("resource '%s' is invalid", k.Resource) - } - if k.Name != "" && !validNameRegex.MatchString(k.Name) { - return fmt.Errorf("name '%s' is invalid", k.Name) + if err := validation.IsValidResource(k.Resource); err != nil { + return NewValidationError("resource", k.Resource, err[0]) } + return nil } @@ -168,31 +150,20 @@ type GetRequestKey struct { // Validate validates the get request key func (k GetRequestKey) Validate() error { - if k.Group == "" { - return fmt.Errorf("group is required") - } - if k.Resource == "" { - return fmt.Errorf("resource is required") - } if k.Namespace == "" { - return fmt.Errorf("namespace is required") + return errors.New(ErrNamespaceRequired) } - if k.Name == "" { - return fmt.Errorf("name is required") + if err := validation.IsValidNamespace(k.Namespace); err != nil { + return NewValidationError("namespace", k.Namespace, err[0]) } - - // Validate naming conventions - if !validNameRegex.MatchString(k.Namespace) { - return fmt.Errorf("namespace '%s' is invalid", k.Namespace) + if err := validation.IsValidGroup(k.Group); err != nil { + return NewValidationError("group", k.Group, err[0]) } - if !validNameRegex.MatchString(k.Group) { - return fmt.Errorf("group '%s' is invalid", k.Group) + if err := validation.IsValidResource(k.Resource); err != nil { + return NewValidationError("resource", k.Resource, err[0]) } - if !validNameRegex.MatchString(k.Resource) { - return fmt.Errorf("resource '%s' is invalid", k.Resource) - } - if !validNameRegex.MatchString(k.Name) { - return fmt.Errorf("name '%s' is invalid", k.Name) + if err := validation.IsValidGrafanaName(k.Name); err != nil { + return NewValidationError("name", k.Name, err[0]) } return nil diff --git a/pkg/storage/unified/resource/datastore_test.go b/pkg/storage/unified/resource/datastore_test.go index 1bdb97fb949..9b405847a9a 100644 --- a/pkg/storage/unified/resource/datastore_test.go +++ b/pkg/storage/unified/resource/datastore_test.go @@ -3,6 +3,7 @@ package resource import ( "bytes" "context" + "errors" "fmt" "io" "testing" @@ -86,6 +87,7 @@ func TestDataKey_Validate(t *testing.T) { key DataKey expectError bool errorMsg string + errorField string }{ { name: "valid key with created action", @@ -99,6 +101,18 @@ func TestDataKey_Validate(t *testing.T) { }, expectError: false, }, + { + name: "valid - underscore in namespace", + key: DataKey{ + Namespace: "test_namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, { name: "valid key with updated action", key: DataKey{ @@ -124,23 +138,23 @@ func TestDataKey_Validate(t *testing.T) { expectError: false, }, { - name: "valid key with dots and dashes", + name: "valid - name ends with dash", key: DataKey{ - Namespace: "test.namespace-with-dashes", - Group: "test.group-123", - Resource: "test-resource.v1", - Name: "test-name.with.dots", + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name-", ResourceVersion: rv, Action: DataActionCreated, }, expectError: false, }, { - name: "valid key with single character names", + name: "valid key with minimum character lengths", key: DataKey{ - Namespace: "a", - Group: "b", - Resource: "c", + Namespace: "abc", + Group: "bcd", + Resource: "cde", Name: "d", ResourceVersion: rv, Action: DataActionCreated, @@ -159,6 +173,54 @@ func TestDataKey_Validate(t *testing.T) { }, expectError: false, }, + { + name: "valid - uppercase in name", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "Test-Name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - uppercase in namespace", + key: DataKey{ + Namespace: "Test-Namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - uppercase in group", + key: DataKey{ + Namespace: "test-namespace", + Group: "Test-Group", + Resource: "test-resource", + Name: "test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - uppercase in resource", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "Test-Resource", + Name: "test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, // Invalid cases - empty fields { name: "invalid - empty namespace", @@ -171,7 +233,7 @@ func TestDataKey_Validate(t *testing.T) { Action: DataActionCreated, }, expectError: true, - errorMsg: "namespace is required", + errorMsg: ErrNamespaceRequired, }, { name: "invalid - empty group", @@ -184,7 +246,7 @@ func TestDataKey_Validate(t *testing.T) { Action: DataActionCreated, }, expectError: true, - errorMsg: "group is required", + errorField: "group", }, { name: "invalid - empty resource", @@ -197,7 +259,7 @@ func TestDataKey_Validate(t *testing.T) { Action: DataActionCreated, }, expectError: true, - errorMsg: "resource is required", + errorField: "resource", }, { name: "invalid - empty name", @@ -210,7 +272,7 @@ func TestDataKey_Validate(t *testing.T) { Action: DataActionCreated, }, expectError: true, - errorMsg: "name is required", + errorField: "name", }, { name: "invalid - empty action", @@ -223,7 +285,7 @@ func TestDataKey_Validate(t *testing.T) { Action: "", }, expectError: true, - errorMsg: "action is required", + errorMsg: ErrActionRequired, }, { name: "invalid - all fields empty", @@ -236,74 +298,21 @@ func TestDataKey_Validate(t *testing.T) { Action: "", }, expectError: true, - errorMsg: "group is required", - }, - // Invalid cases - uppercase characters - { - name: "invalid - uppercase in namespace", - key: DataKey{ - Namespace: "Test-Namespace", - Group: "test-group", - Resource: "test-resource", - Name: "test-name", - ResourceVersion: rv, - Action: DataActionCreated, - }, - expectError: true, - errorMsg: "namespace 'Test-Namespace' is invalid", - }, - { - name: "invalid - uppercase in group", - key: DataKey{ - Namespace: "test-namespace", - Group: "Test-Group", - Resource: "test-resource", - Name: "test-name", - ResourceVersion: rv, - Action: DataActionCreated, - }, - expectError: true, - errorMsg: "group 'Test-Group' is invalid", - }, - { - name: "invalid - uppercase in resource", - key: DataKey{ - Namespace: "test-namespace", - Group: "test-group", - Resource: "Test-Resource", - Name: "test-name", - ResourceVersion: rv, - Action: DataActionCreated, - }, - expectError: true, - errorMsg: "resource 'Test-Resource' is invalid", - }, - { - name: "invalid - uppercase in name", - key: DataKey{ - Namespace: "test-namespace", - Group: "test-group", - Resource: "test-resource", - Name: "Test-Name", - ResourceVersion: rv, - Action: DataActionCreated, - }, - expectError: true, - errorMsg: "name 'Test-Name' is invalid", + errorField: "namespace", }, // Invalid cases - invalid characters { - name: "invalid - underscore in namespace", + name: "invalid - key with dots and dashes", key: DataKey{ - Namespace: "test_namespace", - Group: "test-group", - Resource: "test-resource", - Name: "test-name", + Namespace: "test.namespace-with-dashes", + Group: "test.group-123", + Resource: "test-resource.v1", + Name: "test-name.with.dots", ResourceVersion: rv, Action: DataActionCreated, }, expectError: true, - errorMsg: "namespace 'test_namespace' is invalid", + errorField: "namespace", }, { name: "invalid - space in group", @@ -331,8 +340,154 @@ func TestDataKey_Validate(t *testing.T) { expectError: true, errorMsg: "resource 'test@resource' is invalid", }, + // Name validation tests - K8s qualified name format { - name: "invalid - slash in name", + name: "valid - K8s format with underscores", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test_name_with_underscores", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - K8s format with dots", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test.name.with.dots", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - K8s format mixed case", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "TestName123", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - Legacy Grafana shortid format", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "a1B2c3D4e5F6g7H8", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - Legacy format with dashes and underscores", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name_with-mixed_chars123", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - Single character name", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "a", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name starts with dash (legacy format)", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "-test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name ends with dash (legacy format)", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name-", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name starts with dot", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: ".test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name ends with dot", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name.", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name starts with underscore (legacy format)", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "_test-name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + { + name: "valid - name ends with underscore (legacy format)", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name_", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: false, + }, + // Invalid name cases + { + name: "invalid - name with slash", key: DataKey{ Namespace: "test-namespace", Group: "test-group", @@ -342,7 +497,46 @@ func TestDataKey_Validate(t *testing.T) { Action: DataActionCreated, }, expectError: true, - errorMsg: "name 'test/name' is invalid", + errorField: "name", + }, + { + name: "invalid - name with spaces", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test name", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: true, + errorField: "name", + }, + { + name: "invalid - name with special characters", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test@name#with$special", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: true, + errorField: "name", + }, + { + name: "invalid - empty name", + key: DataKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "", + ResourceVersion: rv, + Action: DataActionCreated, + }, + expectError: true, + errorField: "name", }, // Invalid cases - start/end with invalid characters { @@ -384,19 +578,6 @@ func TestDataKey_Validate(t *testing.T) { expectError: true, errorMsg: "resource '.test-resource' is invalid", }, - { - name: "invalid - name ends with dash", - key: DataKey{ - Namespace: "test-namespace", - Group: "test-group", - Resource: "test-resource", - Name: "test-name-", - ResourceVersion: rv, - Action: DataActionCreated, - }, - expectError: true, - errorMsg: "name 'test-name-' is invalid", - }, // Invalid cases - invalid action { name: "invalid - unknown action", @@ -421,6 +602,10 @@ func TestDataKey_Validate(t *testing.T) { if tt.errorMsg != "" { require.Contains(t, err.Error(), tt.errorMsg) } + var validationErr *ValidationError + if errors.Is(err, validationErr) && tt.errorField != "" { + require.Equal(t, tt.errorField, validationErr.Field) + } } else { require.NoError(t, err) } @@ -974,7 +1159,7 @@ func TestDataStore_ValidationEnforced(t *testing.T) { // Create an invalid key invalidKey := DataKey{ - Namespace: "Invalid-Namespace", // uppercase is invalid + Namespace: "Invalid-Namespace-$$$", Group: "test-group", Resource: "test-resource", Name: "test-name", @@ -988,21 +1173,27 @@ func TestDataStore_ValidationEnforced(t *testing.T) { _, err := ds.Get(ctx, invalidKey) require.Error(t, err) require.Contains(t, err.Error(), "invalid data key") - require.Contains(t, err.Error(), "namespace 'Invalid-Namespace' is invalid") + var validationErr ValidationError + require.True(t, errors.As(err, &validationErr)) + require.Equal(t, "namespace", validationErr.Field) }) t.Run("Save with invalid key returns validation error", func(t *testing.T) { err := ds.Save(ctx, invalidKey, testValue) require.Error(t, err) require.Contains(t, err.Error(), "invalid data key") - require.Contains(t, err.Error(), "namespace 'Invalid-Namespace' is invalid") + var validationErr ValidationError + require.True(t, errors.As(err, &validationErr)) + require.Equal(t, "namespace", validationErr.Field) }) t.Run("Delete with invalid key returns validation error", func(t *testing.T) { err := ds.Delete(ctx, invalidKey) require.Error(t, err) require.Contains(t, err.Error(), "invalid data key") - require.Contains(t, err.Error(), "namespace 'Invalid-Namespace' is invalid") + var validationErr ValidationError + require.True(t, errors.As(err, &validationErr)) + require.Equal(t, "namespace", validationErr.Field) }) // Test another type of invalid key @@ -1043,6 +1234,7 @@ func TestListRequestKey_Validate(t *testing.T) { key ListRequestKey expectError bool errorMsg string + errorField string }{ { name: "valid - all fields provided", @@ -1054,6 +1246,45 @@ func TestListRequestKey_Validate(t *testing.T) { }, expectError: false, }, + { + name: "valid - uppercase in namespace", + key: ListRequestKey{ + Namespace: "Test-Namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name", + }, + expectError: false, + }, + { + name: "valid - uppercase in group and resource", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "Test-Group", + Resource: "test-resource", + Name: "test-name", + }, + expectError: false, + }, + { + name: "valid - uppercase in resource", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "Test-Resource", + }, + expectError: false, + }, + { + name: "valid - underscore in namespace", + key: ListRequestKey{ + Namespace: "test_namespace", + Group: "test-group", + Resource: "test-resource", + Name: "test-name", + }, + expectError: false, + }, { name: "valid - only group and resource", key: ListRequestKey{ @@ -1075,7 +1306,47 @@ func TestListRequestKey_Validate(t *testing.T) { name: "invalid - all empty", key: ListRequestKey{}, expectError: true, - errorMsg: "group is required", + errorField: "namespace", + }, + { + name: "valid - legacy grafana uid 1", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "_4OV_5Nmz", + }, + expectError: false, + }, + { + name: "valid - legacy grafana uid 2", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "-Y-tnEDWk", + }, + expectError: false, + }, + { + name: "valid - legacy grafana uid 3", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "000000005", + }, + expectError: false, + }, + { + name: "valid - uppercase in name", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group", + Resource: "test-resource", + Name: "Test-Name", + }, + expectError: false, }, // Invalid hierarchical cases { @@ -1084,7 +1355,7 @@ func TestListRequestKey_Validate(t *testing.T) { Group: "test-group", }, expectError: true, - errorMsg: "resource is required", + errorField: "resource", }, { name: "invalid - name without namespace", @@ -1094,7 +1365,7 @@ func TestListRequestKey_Validate(t *testing.T) { Group: "test-group", }, expectError: true, - errorMsg: "name must be empty when namespace is empty", + errorMsg: ErrNameMustBeEmptyWhenNamespaceEmpty, }, { name: "invalid - name without group and resource", @@ -1103,63 +1374,9 @@ func TestListRequestKey_Validate(t *testing.T) { Name: "test-name", }, expectError: true, - errorMsg: "group is required", + errorField: "group", }, // Invalid naming cases - { - name: "invalid - uppercase in namespace", - key: ListRequestKey{ - Namespace: "Test-Namespace", - Group: "test-group", - Resource: "test-resource", - Name: "test-name", - }, - expectError: true, - errorMsg: "namespace 'Test-Namespace' is invalid", - }, - { - name: "invalid - uppercase in group and resource", - key: ListRequestKey{ - Namespace: "test-namespace", - Group: "Test-Group", - Resource: "test-resource", - Name: "test-name", - }, - expectError: true, - errorMsg: "group 'Test-Group' is invalid", - }, - { - name: "invalid - uppercase in resource", - key: ListRequestKey{ - Namespace: "test-namespace", - Group: "test-group", - Resource: "Test-Resource", - }, - expectError: true, - errorMsg: "resource 'Test-Resource' is invalid", - }, - { - name: "invalid - uppercase in name", - key: ListRequestKey{ - Namespace: "test-namespace", - Group: "test-group", - Resource: "test-resource", - Name: "Test-Name", - }, - expectError: true, - errorMsg: "name 'Test-Name' is invalid", - }, - { - name: "invalid - underscore in namespace", - key: ListRequestKey{ - Namespace: "test_namespace", - Group: "test-group", - Resource: "test-resource", - Name: "test-name", - }, - expectError: true, - errorMsg: "namespace 'test_namespace' is invalid", - }, { name: "invalid - starts with dash", key: ListRequestKey{ @@ -1182,6 +1399,16 @@ func TestListRequestKey_Validate(t *testing.T) { expectError: true, errorMsg: "group 'test-group.' is invalid", }, + { + name: "invalid - name contains invalid char", + key: ListRequestKey{ + Namespace: "test-namespace", + Group: "test-group.", + Resource: "test-resource", + Name: "test$name", + }, + expectError: true, + }, } for _, tt := range tests { @@ -2288,10 +2515,11 @@ func TestDataKey_SameResource(t *testing.T) { func TestGetRequestKey_Validate(t *testing.T) { tests := []struct { - name string - key GetRequestKey - expectErr bool - wantError string + name string + key GetRequestKey + expectErr bool + wantError string + errorField string }{ { name: "valid key", @@ -2313,6 +2541,16 @@ func TestGetRequestKey_Validate(t *testing.T) { }, expectErr: false, }, + { + name: "valid grafana name - ends with dot", + key: GetRequestKey{ + Group: "apps", + Resource: "resources", + Namespace: "default", + Name: ".123_hello", + }, + expectErr: false, + }, { name: "missing group", key: GetRequestKey{ @@ -2320,8 +2558,8 @@ func TestGetRequestKey_Validate(t *testing.T) { Namespace: "default", Name: "test-resource", }, - expectErr: true, - wantError: "group is required", + expectErr: true, + errorField: "group", }, { name: "missing resource", @@ -2330,8 +2568,8 @@ func TestGetRequestKey_Validate(t *testing.T) { Namespace: "default", Name: "test-resource", }, - expectErr: true, - wantError: "resource is required", + expectErr: true, + errorField: "resource", }, { name: "missing namespace", @@ -2340,8 +2578,8 @@ func TestGetRequestKey_Validate(t *testing.T) { Resource: "resources", Name: "test-resource", }, - expectErr: true, - wantError: "namespace is required", + expectErr: true, + errorField: "namespace", }, { name: "missing name", @@ -2350,30 +2588,19 @@ func TestGetRequestKey_Validate(t *testing.T) { Resource: "resources", Namespace: "default", }, - expectErr: true, - wantError: "name is required", + expectErr: true, + errorField: "name", }, { - name: "invalid namespace - uppercase", + name: "invalid group - underscore at start", key: GetRequestKey{ - Group: "apps", - Resource: "resources", - Namespace: "Default", - Name: "test-resource", - }, - expectErr: true, - wantError: "namespace 'Default' is invalid", - }, - { - name: "invalid group - underscore", - key: GetRequestKey{ - Group: "apps_v1", + Group: "_apps_v1", Resource: "resources", Namespace: "default", Name: "test-resource", }, - expectErr: true, - wantError: "group 'apps_v1' is invalid", + expectErr: true, + errorField: "group", }, { name: "invalid resource - starts with dash", @@ -2383,19 +2610,8 @@ func TestGetRequestKey_Validate(t *testing.T) { Namespace: "default", Name: "test-resource", }, - expectErr: true, - wantError: "resource '-resources' is invalid", - }, - { - name: "invalid name - ends with dot", - key: GetRequestKey{ - Group: "apps", - Resource: "resources", - Namespace: "default", - Name: "test-resource.", - }, - expectErr: true, - wantError: "name 'test-resource.' is invalid", + expectErr: true, + errorField: "resource", }, } @@ -2407,6 +2623,10 @@ func TestGetRequestKey_Validate(t *testing.T) { if tt.wantError != "" { require.Contains(t, err.Error(), tt.wantError) } + var validationErr *ValidationError + if errors.Is(err, validationErr) && tt.errorField != "" { + require.Equal(t, tt.errorField, validationErr.Field) + } } else { require.NoError(t, err) } diff --git a/pkg/storage/unified/resource/errors.go b/pkg/storage/unified/resource/errors.go index a0402be1dcb..91b1332c684 100644 --- a/pkg/storage/unified/resource/errors.go +++ b/pkg/storage/unified/resource/errors.go @@ -2,6 +2,7 @@ package resource import ( "errors" + "fmt" "net/http" "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" @@ -199,3 +200,25 @@ func HandleQueueError[T any](err error, makeResp func(*resourcepb.ErrorResult) * } return makeResp(AsErrorResult(err)), nil } + +var ( + ErrNamespaceRequired = "namespace is required" + ErrResourceVersionInvalid = "resource version must be positive" + ErrActionRequired = "action is required" + ErrActionInvalid = "action is invalid: must be one of 'created', 'updated', or 'deleted'" + ErrNameMustBeEmptyWhenNamespaceEmpty = "name must be empty when namespace is empty" +) + +type ValidationError struct { + Field string + Value string + Msg string +} + +func (e ValidationError) Error() string { + return fmt.Sprintf("%s '%s' is invalid: %s", e.Field, e.Value, e.Msg) +} + +func NewValidationError(field, value, msg string) error { + return ValidationError{Field: field, Value: value, Msg: msg} +} diff --git a/pkg/storage/unified/resource/eventstore.go b/pkg/storage/unified/resource/eventstore.go index 6558a81839c..543849efe9f 100644 --- a/pkg/storage/unified/resource/eventstore.go +++ b/pkg/storage/unified/resource/eventstore.go @@ -3,6 +3,7 @@ package resource import ( "context" "encoding/json" + "errors" "fmt" "iter" "strconv" @@ -10,6 +11,7 @@ import ( "time" "github.com/bwmarrin/snowflake" + "github.com/grafana/grafana/pkg/apimachinery/validation" ) const ( @@ -37,46 +39,38 @@ func (k EventKey) String() string { func (k EventKey) Validate() error { if k.Namespace == "" { - return fmt.Errorf("namespace cannot be empty") - } - if k.Group == "" { - return fmt.Errorf("group cannot be empty") - } - if k.Resource == "" { - return fmt.Errorf("resource cannot be empty") - } - if k.Name == "" { - return fmt.Errorf("name cannot be empty") + return NewValidationError("namespace", k.Namespace, ErrNamespaceRequired) } if k.ResourceVersion < 0 { - return fmt.Errorf("resource version must be non-negative") + return errors.New(ErrResourceVersionInvalid) } if k.Action == "" { - return fmt.Errorf("action cannot be empty") + return NewValidationError("action", string(k.Action), ErrActionRequired) } - if k.Folder != "" && !validNameRegex.MatchString(k.Folder) { - return fmt.Errorf("folder '%s' is invalid", k.Folder) + + // Validate each field against the naming rules + // Validate naming conventions for all required fields + if err := validation.IsValidNamespace(k.Namespace); err != nil { + return NewValidationError("namespace", k.Namespace, err[0]) } - // Validate each field against the naming rules (reusing the regex from datastore.go) - if !validNameRegex.MatchString(k.Namespace) { - return fmt.Errorf("namespace '%s' is invalid", k.Namespace) + if err := validation.IsValidGroup(k.Group); err != nil { + return NewValidationError("group", k.Group, err[0]) } - if !validNameRegex.MatchString(k.Group) { - return fmt.Errorf("group '%s' is invalid", k.Group) + if err := validation.IsValidResource(k.Resource); err != nil { + return NewValidationError("resource", k.Resource, err[0]) } - if !validNameRegex.MatchString(k.Resource) { - return fmt.Errorf("resource '%s' is invalid", k.Resource) + if err := validation.IsValidGrafanaName(k.Name); err != nil { + return NewValidationError("name", k.Name, err[0]) } - if !validNameRegex.MatchString(k.Name) { - return fmt.Errorf("name '%s' is invalid", k.Name) - } - if k.Folder != "" && !validNameRegex.MatchString(k.Folder) { - return fmt.Errorf("folder '%s' is invalid", k.Folder) + if k.Folder != "" { + if err := validation.IsValidGrafanaName(k.Folder); err != nil { + return NewValidationError("folder", k.Folder, err[0]) + } } switch k.Action { case DataActionCreated, DataActionUpdated, DataActionDeleted: default: - return fmt.Errorf("action '%s' is invalid: must be one of 'created', 'updated', or 'deleted'", k.Action) + return NewValidationError("action", string(k.Action), ErrActionInvalid) } return nil @@ -148,6 +142,7 @@ func (n *eventStore) Save(ctx context.Context, event Event) error { Name: event.Name, ResourceVersion: event.ResourceVersion, Action: event.Action, + //TODO why isnt folder part of the key? } if err := eventKey.Validate(); err != nil { diff --git a/pkg/storage/unified/resource/keys.go b/pkg/storage/unified/resource/keys.go index 324a2128633..8c9b4b42a93 100644 --- a/pkg/storage/unified/resource/keys.go +++ b/pkg/storage/unified/resource/keys.go @@ -41,7 +41,7 @@ func verifyRequestKeyNamespaceGroupResource(key *resourcepb.ResourceKey) *resour if err := validation.IsValidGroup(key.Group); err != nil { return NewBadRequestError(err[0]) } - if err := validation.IsValidateResource(key.Resource); err != nil { + if err := validation.IsValidResource(key.Resource); err != nil { return NewBadRequestError(err[0]) } return nil diff --git a/pkg/storage/unified/resource/kv.go b/pkg/storage/unified/resource/kv.go index 613841c5994..a4215a1570a 100644 --- a/pkg/storage/unified/resource/kv.go +++ b/pkg/storage/unified/resource/kv.go @@ -259,9 +259,9 @@ func PrefixRangeEnd(prefix string) string { var ( // validKeyRegex validates keys used in the unified storage - // Keys can contain lowercase alphanumeric characters, '-', '.', '/', and '~' + // Keys can contain alphanumeric characters (both upper and lowercase), '-', '.', '/', and '~' // Any combination of these characters is allowed as long as the key is not empty - validKeyRegex = regexp.MustCompile(`^[a-z0-9./~-]+$`) + validKeyRegex = regexp.MustCompile(`^[a-zA-Z0-9./~_-]+$`) ) func IsValidKey(key string) bool { diff --git a/pkg/storage/unified/resource/kv_test.go b/pkg/storage/unified/resource/kv_test.go index 27f24e19122..b5625405b73 100644 --- a/pkg/storage/unified/resource/kv_test.go +++ b/pkg/storage/unified/resource/kv_test.go @@ -235,17 +235,19 @@ func TestIsValidKey(t *testing.T) { {"data key format", "ns/group/resource/name/123~created", true}, {"metadata key format", "group/resource/ns/name/123~created~folder", true}, {"metadata key format ending with a ~", "group/resource/ns/name/123~created~", true}, + {"uppercase letters", "Valid", true}, + {"underscores", "a_b", true}, + {"key with underscores and mixed chars", "4_D6mSh4z", true}, + {"complex key with underscores", "ns/group_name/resource-name/Name_123~action-type", true}, // invalid keys {"empty key", "", false}, - {"uppercase letters", "Invalid", false}, {"special characters", "a@b", false}, {"spaces", "a b", false}, {"leading space", " key", false}, {"trailing space", "key ", false}, {"tab character", "a\tb", false}, {"newline character", "a\nb", false}, - {"underscores", "a_b", false}, } for _, tt := range tests { diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 1a304d49f68..59ae4e1dc83 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -554,7 +554,7 @@ 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 errs := validation.IsValidGrafanaName(obj.GetName()); err != nil { + if errs := validation.IsValidGrafanaName(obj.GetName()); errs != nil { return nil, NewBadRequestError(errs[0]) }