public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Dashboards: Add Dashboard Schema validation (2) (#103844)

Browse Source 
* Activate schema validation and align underlying systems

* update to save as v0 if not the right schema version

* Resolve merge conflicts

* Move RequireApiErrorStatus to tests package

* Add mutation tests

* Fix lint

* Only do min version check if dashboard is v1

* Fix lint and disable provisioning test

* Revert provisioning changes

* Revert more tests and add schema test

* Reran gen

* SQL Dashboard save

* Adjust APIVERSION

* Fixed mutation test

* Add logging on downgrade

---------

Co-authored-by: Marco de Abreu <18629099+marcoabreu@users.noreply.github.com>
Co-authored-by: Stephanie Hingtgen <stephanie.hingtgen@grafana.com>
This commit is contained in:
Marco de Abreu
2025-04-11 23:05:41 +02:00
committed by GitHub
co-authored by Marco de Abreu Stephanie Hingtgen
parent 07a225649d
commit c47ab101d1
21 changed files with 500 additions and 128 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/registry/apis/dashboard/legacy/sql_dashboards.go
+14
View File
@@ -15,11 +15,14 @@ import (
claims "github.com/grafana/authlib/types"
dashboardOG "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard"
dashboardv0 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v0alpha1"
dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1alpha1"
"github.com/grafana/grafana/apps/dashboard/pkg/migration/schemaversion"
"github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/components/simplejson"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/registry/apis/dashboard/legacysearcher"
"github.com/grafana/grafana/pkg/services/apiserver/endpoints/request"
gapiutil "github.com/grafana/grafana/pkg/services/apiserver/utils"
@@ -62,6 +65,7 @@ type dashboardSqlAccess struct {
// Typically one... the server wrapper
subscribers []chan *resource.WrittenEvent
mutex sync.Mutex
log log.Logger
}
func NewDashboardAccess(sql legacysql.LegacyDatabaseProvider,
@@ -77,6 +81,7 @@ func NewDashboardAccess(sql legacysql.LegacyDatabaseProvider,
dashStore: dashStore,
provisioning: provisioning,
dashboardSearchClient: *dashboardSearchClient,
log: log.New("dashboard.legacysql"),
}
}
@@ -410,6 +415,15 @@ func (a *dashboardSqlAccess) buildSaveDashboardCommand(ctx context.Context, orgI
}
}
// v1 should be saved as schema version 41. v0 allows for older versions
if strings.HasSuffix(dash.APIVersion, "v1alpha1") {
schemaVersion := schemaversion.GetSchemaVersion(dash.Spec.Object)
if schemaVersion < int(schemaversion.LATEST_VERSION) {
dash.APIVersion = dashboardv0.VERSION
a.log.Info("Downgrading v1alpha1 dashboard to v0alpha1 due to schema version mismatch", "dashboard", dash.Name, "schema_version", schemaVersion)
}
}
apiVersion := strings.TrimPrefix(dash.APIVersion, dashboard.GROUP+"/")
meta, err := utils.MetaAccessor(dash)
if err != nil {
pkg/registry/apis/dashboard/legacy/sql_dashboards_test.go
+89 -52
View File
@@ -14,6 +14,7 @@ import (
common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/provisioning"
"github.com/grafana/grafana/pkg/services/user"
@@ -30,6 +31,7 @@ func TestScanRow(t *testing.T) {
store := &dashboardSqlAccess{
namespacer: func(_ int64) string { return "default" },
provisioning: provisioner,
log: log.New("test"),
}
columns := []string{"orgId", "dashboard_id", "name", "folder_uid", "deleted", "plugin_id", "origin_name", "origin_path", "origin_hash", "origin_ts", "created", "createdBy", "createdByID", "updated", "updatedBy", "updatedByID", "version", "message", "data", "api_version"}
@@ -126,60 +128,95 @@ func TestScanRow(t *testing.T) {
}
func TestBuildSaveDashboardCommand(t *testing.T) {
mockStore := &dashboards.FakeDashboardStore{}
access := &dashboardSqlAccess{
dashStore: mockStore,
testCases := []struct {
name string
schemaVersion int
expectedAPI string
}{
{
name: "with schema version 36 should save as v0alpha1",
schemaVersion: 36,
expectedAPI: "v0alpha1",
},
{
name: "with schema version 41 should save as v1alpha1",
schemaVersion: 41,
expectedAPI: "v1alpha1",
},
{
name: "with empty schema version should save as v0alpha1",
schemaVersion: 0,
expectedAPI: "v0alpha1",
},
}
dash := &dashboard.Dashboard{
TypeMeta: metav1.TypeMeta{
APIVersion: dashboard.APIVERSION,
},
ObjectMeta: metav1.ObjectMeta{
Name: "test-dash",
},
Spec: common.Unstructured{
Object: map[string]interface{}{
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockStore := &dashboards.FakeDashboardStore{}
access := &dashboardSqlAccess{
dashStore: mockStore,
log: log.New("test"),
}
dashSpec := map[string]interface{}{
"title": "Test Dashboard",
"id": 123,
},
},
}
if tc.schemaVersion > 0 {
dashSpec["schemaVersion"] = tc.schemaVersion
}
dash := &dashboard.Dashboard{
TypeMeta: metav1.TypeMeta{
APIVersion: dashboard.APIVERSION,
},
ObjectMeta: metav1.ObjectMeta{
Name: "test-dash",
},
Spec: common.Unstructured{
Object: dashSpec,
},
}
// fail if no user in context
_, _, err := access.buildSaveDashboardCommand(context.Background(), 1, dash)
require.Error(t, err)
ctx := identity.WithRequester(context.Background(), &user.SignedInUser{
OrgID: 1,
OrgRole: "Admin",
})
// create new dashboard
mockStore.On("GetDashboard", mock.Anything, mock.Anything).Return(nil, nil).Once()
cmd, created, err := access.buildSaveDashboardCommand(ctx, 1, dash)
require.NoError(t, err)
require.Equal(t, true, created)
require.NotNil(t, cmd)
require.Equal(t, "test-dash", cmd.Dashboard.Get("uid").MustString())
_, exists := cmd.Dashboard.CheckGet("id")
require.False(t, exists) // id should be removed
require.Equal(t, cmd.OrgID, int64(1))
require.True(t, cmd.Overwrite)
require.Equal(t, tc.expectedAPI, cmd.APIVersion) // verify expected API version
// now update existing dashboard
mockStore.On("GetDashboard", mock.Anything, mock.Anything).Return(
&dashboards.Dashboard{
ID: 1234,
Version: 2,
APIVersion: dashboard.APIVERSION,
}, nil).Once()
cmd, created, err = access.buildSaveDashboardCommand(ctx, 1, dash)
require.NoError(t, err)
require.Equal(t, false, created)
require.NotNil(t, cmd)
require.Equal(t, "test-dash", cmd.Dashboard.Get("uid").MustString())
require.Equal(t, cmd.Dashboard.Get("id").MustInt64(), int64(1234)) // should set to existing ID
require.Equal(t, cmd.Dashboard.Get("version").MustFloat64(), float64(2)) // version must be set - otherwise seen as a new dashboard in NewDashboardFromJson
require.Equal(t, tc.expectedAPI, cmd.APIVersion) // verify expected API version
require.Equal(t, cmd.OrgID, int64(1))
require.True(t, cmd.Overwrite)
})
}
// fail if no user in context
_, _, err := access.buildSaveDashboardCommand(context.Background(), 1, dash)
require.Error(t, err)
ctx := identity.WithRequester(context.Background(), &user.SignedInUser{
OrgID: 1,
OrgRole: "Admin",
})
// create new dashboard
mockStore.On("GetDashboard", mock.Anything, mock.Anything).Return(nil, nil).Once()
cmd, created, err := access.buildSaveDashboardCommand(ctx, 1, dash)
require.NoError(t, err)
require.Equal(t, true, created)
require.NotNil(t, cmd)
require.Equal(t, "test-dash", cmd.Dashboard.Get("uid").MustString())
_, exists := cmd.Dashboard.CheckGet("id")
require.False(t, exists) // id should be removed
require.Equal(t, cmd.OrgID, int64(1))
require.True(t, cmd.Overwrite)
// now update existing dashboard
mockStore.On("GetDashboard", mock.Anything, mock.Anything).Return(
&dashboards.Dashboard{
ID: 1234,
Version: 2,
APIVersion: dashboard.APIVERSION,
}, nil).Once()
cmd, created, err = access.buildSaveDashboardCommand(ctx, 1, dash)
require.NoError(t, err)
require.Equal(t, false, created)
require.NotNil(t, cmd)
require.Equal(t, "test-dash", cmd.Dashboard.Get("uid").MustString())
require.Equal(t, cmd.Dashboard.Get("id").MustInt64(), int64(1234)) // should set to existing ID
require.Equal(t, cmd.Dashboard.Get("version").MustFloat64(), float64(2)) // version must be set - otherwise seen as a new dashboard in NewDashboardFromJson
require.Equal(t, cmd.APIVersion, "v1alpha1") // should trim prefix
require.Equal(t, cmd.OrgID, int64(1))
require.True(t, cmd.Overwrite)
}
pkg/registry/apis/dashboard/legacy/storage_test.go
+2
View File
@@ -12,6 +12,7 @@ import (
dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1alpha1"
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/storage/unified/resource"
@@ -119,6 +120,7 @@ func TestWriteProvisioningEvent(t *testing.T) {
access := &dashboardSqlAccess{
dashStore: mockStore,
log: log.New("test"),
}
ctx := identity.WithRequester(context.Background(), &user.SignedInUser{})
pkg/registry/apis/dashboard/mutate.go
+10
View File
@@ -58,7 +58,17 @@ func (b *DashboardsAPIBuilder) Mutate(ctx context.Context, a admission.Attribute
}
}
case *dashboardV2.Dashboard:
// Temporary fix: The generator fails to properly initialize this property, so we'll do it here
// until the generator is fixed.
if v.Spec.Layout.GridLayoutKind == nil && v.Spec.Layout.RowsLayoutKind == nil && v.Spec.Layout.AutoGridLayoutKind == nil && v.Spec.Layout.TabsLayoutKind == nil {
v.Spec.Layout.GridLayoutKind = &dashboardV2.DashboardGridLayoutKind{
Kind: "GridLayout",
Spec: dashboardV2.DashboardGridLayoutSpec{},
}
}
resourceInfo = dashboardV2.DashboardResourceInfo
// Noop for V2
default:
return fmt.Errorf("mutation error: expected to dashboard, got %T", obj)
pkg/registry/apis/dashboard/mutation_test.go
+94 -8
View File
@@ -6,10 +6,13 @@ import (
dashv0 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v0alpha1"
dashv1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1alpha1"
"github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v2alpha1"
"github.com/grafana/grafana/apps/dashboard/pkg/migration/schemaversion"
common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/admission"
@@ -17,12 +20,14 @@ import (
func TestDashboardAPIBuilder_Mutate(t *testing.T) {
tests := []struct {
name string
inputObj runtime.Object
operation admission.Operation
expectedID int64
migrationExpected bool
expectedError bool
name string
inputObj runtime.Object
operation admission.Operation
expectedID int64
migrationExpected bool
expectedTitle string
expectedError bool
fieldValidationMode string
}{
{
name: "should skip non-create/update operations",
@@ -48,6 +53,47 @@ func TestDashboardAPIBuilder_Mutate(t *testing.T) {
operation: admission.Create,
expectedID: 123,
},
{
name: "v0 should not fail with invalid schema",
inputObj: &dashv0.Dashboard{
Spec: common.Unstructured{
Object: map[string]interface{}{
"id": float64(123),
"revision": "revision-is-a-number",
},
},
},
operation: admission.Create,
expectedID: 123,
},
{
name: "v1 should fail with invalid schema",
inputObj: &dashv1.Dashboard{
Spec: common.Unstructured{
Object: map[string]interface{}{
"id": float64(123),
"revision": "revision-is-a-number",
},
},
},
operation: admission.Create,
expectedError: true,
},
{
name: "v1 should not fail with invalid schema and FieldValidationIgnore is set",
inputObj: &dashv1.Dashboard{
Spec: common.Unstructured{
Object: map[string]interface{}{
"id": float64(123),
"revision": "revision-is-a-number",
},
},
},
operation: admission.Create,
fieldValidationMode: metav1.FieldValidationIgnore,
expectedError: false,
expectedID: 123,
},
{
name: "v1 should migrate dashboard to the latest version, if possible, and set as label",
inputObj: &dashv1.Dashboard{
@@ -75,11 +121,47 @@ func TestDashboardAPIBuilder_Mutate(t *testing.T) {
operation: admission.Create,
expectedError: true,
},
{
name: "v1 should not error mutation hook if migration fails and FieldValidationIgnore is set",
inputObj: &dashv1.Dashboard{
Spec: common.Unstructured{
Object: map[string]interface{}{
"id": float64(456),
"schemaVersion": schemaversion.MIN_VERSION - 1,
},
},
},
expectedID: 456,
operation: admission.Create,
fieldValidationMode: metav1.FieldValidationIgnore,
expectedError: false,
},
{
name: "v2 should set layout if it is not set",
inputObj: &v2alpha1.Dashboard{
Spec: v2alpha1.DashboardSpec{
Title: "test123",
},
},
operation: admission.Create,
expectedTitle: "test123",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
b := &DashboardsAPIBuilder{}
b := &DashboardsAPIBuilder{
features: featuremgmt.WithFeatures(),
}
var operationOptions runtime.Object
switch tt.operation {
case admission.Create:
operationOptions = &metav1.CreateOptions{FieldValidation: tt.fieldValidationMode}
case admission.Update:
operationOptions = &metav1.UpdateOptions{FieldValidation: tt.fieldValidationMode}
default:
operationOptions = nil
}
err := b.Mutate(context.Background(), admission.NewAttributesRecord(
tt.inputObj,
nil,
@@ -89,7 +171,7 @@ func TestDashboardAPIBuilder_Mutate(t *testing.T) {
schema.GroupVersionResource{},
"",
tt.operation,
nil,
operationOptions,
false,
nil,
), nil)
@@ -117,6 +199,10 @@ func TestDashboardAPIBuilder_Mutate(t *testing.T) {
if tt.migrationExpected {
require.Equal(t, schemaversion.LATEST_VERSION, schemaVersion, "dashboard should be migrated to the latest version")
}
case *v2alpha1.Dashboard:
require.Equal(t, tt.expectedTitle, v.Spec.Title, "title should be set")
require.NotNil(t, v.Spec.Layout, "layout should be set")
require.NotNil(t, v.Spec.Layout.GridLayoutKind, "layout should be a GridLayout")
}
}
})
pkg/registry/apis/dashboard/schema_validation.go
+3 -8
View File
@@ -3,7 +3,6 @@ package dashboard
import (
"context"
_ "embed"
"errors"
"fmt"
"k8s.io/apimachinery/pkg/runtime"
@@ -14,16 +13,12 @@ import (
"github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v2alpha1"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/services/featuremgmt"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
// ValidateDashboardSpec validates the dashboard spec and throws a detailed error if there are validation errors.
func (b *DashboardsAPIBuilder) ValidateDashboardSpec(ctx context.Context, obj runtime.Object, fieldValidationMode string) (field.ErrorList, error) {
// This will be removed with the other PR
return nil, nil
// Unreachable code is intentional until the code above is removed
//nolint:govet
accessor, err := utils.MetaAccessor(obj)
if err != nil {
return nil, fmt.Errorf("error getting meta accessor: %w", err)
@@ -40,11 +35,11 @@ func (b *DashboardsAPIBuilder) ValidateDashboardSpec(ctx context.Context, obj ru
case *v2alpha1.Dashboard:
errorOnSchemaMismatches = !b.features.IsEnabled(ctx, featuremgmt.FlagDashboardDisableSchemaValidationV2)
default:
return nil, fmt.Errorf("Invalid dashboard type: %T", obj)
return nil, fmt.Errorf("invalid dashboard type: %T", obj)
}
}
if mode == metav1.FieldValidationWarn {
return nil, errors.New("FieldValidationWarn is not supported")
return nil, apierrors.NewBadRequest("Not supported: FieldValidationMode: Warn")
}
alwaysLogSchemaValidationErrors := b.features.IsEnabled(ctx, featuremgmt.FlagDashboardSchemaValidationLogging)
pkg/registry/apis/provisioning/resources/dualwriter.go
+7 -3
View File
@@ -38,17 +38,21 @@ func (r *DualReadWriter) Read(ctx context.Context, path string, ref string) (*Pa
info, err := r.repo.Read(ctx, path, ref)
if err != nil {
return nil, fmt.Errorf("read file: %w", err)
_, ok := utils.ExtractApiErrorStatus(err)
if ok {
return nil, err
}
return nil, fmt.Errorf("Read file failed: %w", err)
}
parsed, err := r.parser.Parse(ctx, info)
if err != nil {
return nil, fmt.Errorf("parse file: %w", err)
return nil, apierrors.NewBadRequest(fmt.Sprintf("Parse file failed: %v", err))
}
// Fail as we use the dry run for this response and it's not about updating the resource
if err := parsed.DryRun(ctx); err != nil {
return nil, fmt.Errorf("run dry run: %w", err)
return nil, apierrors.NewBadRequest(fmt.Sprintf("Dry run failed: %v", err))
}
// Authorize based on the existing resource
pkg/registry/apis/provisioning/resources/fileformat.go
+5 -2
View File
@@ -19,8 +19,11 @@ import (
)
var (
ErrUnableToReadResourceBytes = errors.New("unable to read bytes as a resource")
ErrClassicResourceIsAlreadyK8sForm = errors.New("classic resource is already structured with apiVersion and kind")
ErrUnableToReadResourceBytes = errors.New("unable to read bytes as a resource")
ErrUnableToReadPanelsMissing = errors.New("panels property is required")
ErrUnableToReadSchemaVersionMissing = errors.New("schemaVersion property is required")
ErrUnableToReadTagsMissing = errors.New("tags property is required")
ErrClassicResourceIsAlreadyK8sForm = errors.New("classic resource is already structured with apiVersion and kind")
)
// This reads a "classic" file format and will convert it to an unstructured k8s resource