wip: start to use validator in the builder instead of validating on the store hooks

pkg/registry/apis/collections/datasources.go

@@ -1,77 +0,0 @@
-package collections
-
-import (
-	"context"
-	"fmt"
-
-	collections "github.com/grafana/grafana/apps/collections/pkg/apis/collections/v1alpha1"
-	grafanarest "github.com/grafana/grafana/pkg/apiserver/rest"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/util/validation/field"
-	"k8s.io/apiserver/pkg/registry/rest"
-)
-
-var _ grafanarest.Storage = (*datasourceStorage)(nil)
-
-type datasourceStorage struct {
-	grafanarest.Storage
-}
-
-func (s *datasourceStorage) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
-	// TODO run our own validation here
-
-	dsStack, ok := obj.(*collections.DataSourceStack)
-	if !ok {
-		return nil, fmt.Errorf("expected a datasource stack object")
-	}
-
-	list := field.ErrorList{}
-
-	// Check that the modes are valid
-
-	// get the keys from the template
-	template := dsStack.Spec.Template
-	templateKeys := make([]string, 0, len(template))
-	for key := range template {
-		templateKeys = append(templateKeys, key)
-	}
-
-	// for each mode, check that the keys are in the template
-	modes := dsStack.Spec.Modes
-
-	// if a key is not in the template, return an error
-	for _, mode := range modes {
-		for key := range mode.Definition {
-			if indexOf(templateKeys, key) == -1 {
-				list = append(list, field.Invalid(field.NewPath("spec", "modes", mode.Name, "definition", key), key, fmt.Sprintf("key %s is not in the template", key)))
-
-			}
-		}
-	}
-
-	if len(list) > 0 {
-		return nil, apierrors.NewInvalid(collections.DatasourceStacksResourceInfo.GroupVersionKind().GroupKind(), dsStack.Name, list)
-	}
-
-	// TODO find any keys missing from the definition, or is that OK?
-
-	// TODO Check that each data source reference is valid
-
-	return s.Storage.Create(ctx, obj, createValidation, options)
-}
-
-func (d *datasourceStorage) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
-	// the objInfo is not obviosuly simply to perform validation on, it feels like we should be performing validation elsewhere.
-	return d.Storage.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options)
-}
-
-func indexOf(slice []string, item string) int {
-	for i, v := range slice {
-		if v == item {
-			return i
-		}
-	}
-	return -1
-}

pkg/registry/apis/collections/datasources_validator.go

@@ -0,0 +1,75 @@
+package collections
+
+import (
+	"context"
+	"fmt"
+
+	collections "github.com/grafana/grafana/apps/collections/pkg/apis/collections/v1alpha1"
+	"github.com/grafana/grafana/pkg/services/apiserver/builder"
+	"k8s.io/apiserver/pkg/admission"
+)
+
+var _ builder.APIGroupValidation = (*DatasourceStacksValidator)(nil)
+
+type DatasourceStacksValidator struct{}
+
+func GetDatasourceStacksValidator() builder.APIGroupValidation {
+	return &DatasourceStacksValidator{}
+}
+
+func (v *DatasourceStacksValidator) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) (err error) {
+	if a.GetKind().Kind != collections.DatasourceStacksResourceInfo.GroupVersionKind().Kind {
+		return nil
+	}
+
+	obj := a.GetObject()
+	if obj == nil {
+		return fmt.Errorf("object is nil (%s %s)", a.GetName(), a.GetKind().GroupVersion().String())
+	}
+
+	operation := a.GetOperation()
+
+	if operation != admission.Create && operation != admission.Update {
+		return nil
+	}
+
+	cast, ok := obj.(*collections.DataSourceStack)
+	if !ok {
+		return fmt.Errorf("object is not of type *collections.DataSourceStack (%s %s)", a.GetName(), a.GetKind().GroupVersion().String())
+	}
+
+	// get the keys from the template
+	template := cast.Spec.Template
+	templateKeys := map[string]bool{}
+
+	// keys must be unique
+	for key := range template {
+		if _, ok := templateKeys[key]; ok {
+			return fmt.Errorf("template keys must be unique. key '%s' already exists (%s %s)", key, a.GetName(), a.GetKind().GroupVersion().String())
+		}
+		templateKeys[key] = true
+	}
+
+	// template names must be unique
+	templateNames := map[string]bool{}
+	for name := range template {
+		if _, ok := templateNames[name]; ok {
+			return fmt.Errorf("template names must be unique. name '%s' already exists (%s %s)", name, a.GetName(), a.GetKind().GroupVersion().String())
+		}
+		templateNames[name] = true
+	}
+
+	// for each mode, check that the keys are in the template
+	modes := cast.Spec.Modes
+
+	// if a key is not in the template, return an error
+	for _, mode := range modes {
+		for key := range mode.Definition {
+			if _, ok := templateKeys[key]; !ok {
+				return fmt.Errorf("key %s is not in the DataSourceStack template. The template keys are %v (%s %s)", key, templateKeys, a.GetName(), a.GetKind().GroupVersion().String())
+			}
+		}
+	}
+
+	return nil
+}

pkg/registry/apis/collections/datasources_validator_test.go

@@ -0,0 +1,79 @@
+package collections_test
+
+import (
+	"context"
+	"testing"
+
+	collectionsv1alpha1 "github.com/grafana/grafana/apps/collections/pkg/apis/collections/v1alpha1"
+	"github.com/grafana/grafana/pkg/registry/apis/collections"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/admission"
+)
+
+func TestDataSourceValidator_Validate(t *testing.T) {
+	validator := &collections.DatasourceStacksValidator{}
+	ctx := context.Background()
+
+	tests := []struct {
+		name        string
+		operation   admission.Operation
+		object      runtime.Object
+		expectError bool
+		errorMsg    string
+	}{
+		{
+			name:        "should return no error for invalid kind",
+			operation:   admission.Delete,
+			object:      &collectionsv1alpha1.Stars{},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			attrs := &FakeAdmissionAttributes{
+				Operation: tt.operation,
+				Object:    tt.object,
+				Name:      "test-datasourcestack",
+				Kind:      schema.GroupVersionKind{Group: "collections.grafana.app", Version: "v1alpha1", Kind: "DataSourceStack"},
+			}
+
+			err := validator.Validate(ctx, attrs, nil)
+
+			if tt.expectError {
+				assert.Error(t, err)
+				if tt.errorMsg != "" {
+					assert.Contains(t, err.Error(), tt.errorMsg)
+				}
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+type FakeAdmissionAttributes struct {
+	admission.Attributes
+	Operation admission.Operation
+	Object    runtime.Object
+	Name      string
+	Kind      schema.GroupVersionKind
+}
+
+func (m *FakeAdmissionAttributes) GetOperation() admission.Operation {
+	return m.Operation
+}
+
+func (m *FakeAdmissionAttributes) GetObject() runtime.Object {
+	return m.Object
+}
+
+func (m *FakeAdmissionAttributes) GetName() string {
+	return m.Name
+}
+
+func (m *FakeAdmissionAttributes) GetKind() schema.GroupVersionKind {
+	return m.Kind
+}

pkg/registry/apis/collections/register.go

@@ -7,6 +7,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/registry/rest"
 	genericapiserver "k8s.io/apiserver/pkg/server"
@@ -31,13 +32,15 @@ import (
 )
 
 var (
-	_ builder.APIGroupBuilder  = (*APIBuilder)(nil)
+	_ builder.APIGroupBuilder    = (*APIBuilder)(nil)
-	_ builder.APIGroupMutation = (*APIBuilder)(nil)
+	_ builder.APIGroupMutation   = (*APIBuilder)(nil)
+	_ builder.APIGroupValidation = (*APIBuilder)(nil)
 )
 
 type APIBuilder struct {
-	authorizer  authorizer.Authorizer
+	authorizer                authorizer.Authorizer
-	legacyStars *legacy.DashboardStarsStorage
+	legacyStars               *legacy.DashboardStarsStorage
+	datasourceStacksValidator builder.APIGroupValidation
 }
 
 func RegisterAPIService(
@@ -56,6 +59,7 @@ func RegisterAPIService(
 
 	sql := legacy.NewLegacySQL(legacysql.NewDatabaseProvider(db))
 	builder := &APIBuilder{
+		datasourceStacksValidator: GetDatasourceStacksValidator(),
 		authorizer: &utils.AuthorizeFromName{
 			Resource: map[string][]utils.ResourceOwner{
 				"stars":       {utils.UserResourceOwner},
@@ -116,16 +120,19 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI
 	// no need for dual writer for a kind that does not exist in the legacy database
 	resourceInfo := collections.DatasourceStacksResourceInfo
 	datasourcesStorage, err := grafanaregistry.NewRegistryStore(opts.Scheme, resourceInfo, opts.OptsGetter)
-	datasources := &datasourceStorage{Storage: datasourcesStorage}
+	storage[resourceInfo.StoragePath()] = datasourcesStorage
-	if err != nil {
-		return err
-	}
-	storage[resourceInfo.StoragePath()] = datasources
 
 	apiGroupInfo.VersionedResourcesStorageMap[collections.APIVersion] = storage
 	return nil
 }
 
+func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) (err error) {
+	if a.GetKind().Group == collections.DatasourceStacksResourceInfo.GroupResource().Group {
+		return b.datasourceStacksValidator.Validate(ctx, a, o)
+	}
+	return nil
+}
+
 func (b *APIBuilder) GetAuthorizer() authorizer.Authorizer {
 
 	return authorizer.AuthorizerFunc(