K8s: Dashboards: Add deletion validation for provisioned dashboards (#98504)

pkg/registry/apis/dashboard/register.go

@@ -1,18 +1,26 @@
 package dashboard
 
 import (
+	"context"
+	"errors"
+	"fmt"
+
+	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"
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	"k8s.io/kube-openapi/pkg/common"
 	"k8s.io/kube-openapi/pkg/spec3"
 
+	"github.com/grafana/authlib/claims"
 	dashboardinternal "github.com/grafana/grafana/pkg/apis/dashboard"
 	dashboardv0alpha1 "github.com/grafana/grafana/pkg/apis/dashboard/v0alpha1"
 	dashboardv1alpha1 "github.com/grafana/grafana/pkg/apis/dashboard/v1alpha1"
 	dashboardv2alpha1 "github.com/grafana/grafana/pkg/apis/dashboard/v2alpha1"
 	"github.com/grafana/grafana/pkg/services/apiserver/builder"
+	"github.com/grafana/grafana/pkg/services/dashboards"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 )
 
@@ -22,13 +30,18 @@ var (
 )
 
 // This is used just so wire has something unique to return
-type DashboardsAPIBuilder struct{}
+type DashboardsAPIBuilder struct {
+	ProvisioningDashboardService dashboards.DashboardProvisioningService
+}
 
 func RegisterAPIService(
 	features featuremgmt.FeatureToggles,
 	apiregistration builder.APIRegistrar,
+	provisioningDashboardService dashboards.DashboardProvisioningService,
 ) *DashboardsAPIBuilder {
-	builder := &DashboardsAPIBuilder{}
+	builder := &DashboardsAPIBuilder{
+		ProvisioningDashboardService: provisioningDashboardService,
+	}
 	apiregistration.RegisterAPI(builder)
 	return builder
 }
@@ -63,3 +76,37 @@ func (b *DashboardsAPIBuilder) GetOpenAPIDefinitions() common.GetOpenAPIDefiniti
 func (b *DashboardsAPIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAPI, error) {
 	return oas, nil
 }
+
+// Validate will prevent deletion of provisioned dashboards, unless the grace period is set to 0, indicating a force deletion
+func (b *DashboardsAPIBuilder) Validate(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces) (err error) {
+	op := a.GetOperation()
+	if op == admission.Delete {
+		obj := a.GetOperationOptions()
+		deleteOptions, ok := obj.(*metav1.DeleteOptions)
+		if !ok {
+			return fmt.Errorf("expected v1.DeleteOptions")
+		}
+
+		if deleteOptions.GracePeriodSeconds == nil || *deleteOptions.GracePeriodSeconds != 0 {
+			nsInfo, err := claims.ParseNamespace(a.GetNamespace())
+			if err != nil {
+				return fmt.Errorf("%v: %w", "failed to parse namespace", err)
+			}
+
+			provisioningData, err := b.ProvisioningDashboardService.GetProvisionedDashboardDataByDashboardUID(ctx, nsInfo.OrgID, a.GetName())
+			if err != nil {
+				if errors.Is(err, dashboards.ErrProvisionedDashboardNotFound) {
+					return nil
+				}
+
+				return fmt.Errorf("%v: %w", "failed to check if dashboard is provisioned", err)
+			}
+
+			if provisioningData != nil {
+				return dashboards.ErrDashboardCannotDeleteProvisionedDashboard
+			}
+		}
+	}
+
+	return nil
+}

pkg/registry/apis/dashboard/register_test.go

@@ -0,0 +1,160 @@
+package dashboard
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1"
+	"github.com/grafana/grafana/pkg/apis/dashboard/v0alpha1"
+	"github.com/grafana/grafana/pkg/services/dashboards"
+	"github.com/grafana/grafana/pkg/services/user"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apiserver/pkg/admission"
+)
+
+func TestDashboardAPIBuilder_Validate(t *testing.T) {
+	oneInt64 := int64(1)
+	zeroInt64 := int64(0)
+	tests := []struct {
+		name                   string
+		inputObj               *v0alpha1.Dashboard
+		deletionOptions        metav1.DeleteOptions
+		dashboardResponse      *dashboards.DashboardProvisioning
+		dashboardErrorResponse error
+		checkRan               bool
+		expectedError          bool
+	}{
+		{
+			name: "should return an error if data is found",
+			inputObj: &v0alpha1.Dashboard{
+				Spec: common.Unstructured{},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "Dashboard",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			deletionOptions: metav1.DeleteOptions{
+				GracePeriodSeconds: nil,
+			},
+			dashboardResponse:      &dashboards.DashboardProvisioning{ID: 1},
+			dashboardErrorResponse: nil,
+			checkRan:               true,
+			expectedError:          true,
+		},
+		{
+			name: "should return an error if unable to check",
+			inputObj: &v0alpha1.Dashboard{
+				Spec: common.Unstructured{},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "Dashboard",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			deletionOptions: metav1.DeleteOptions{
+				GracePeriodSeconds: nil,
+			},
+			dashboardResponse:      nil,
+			dashboardErrorResponse: fmt.Errorf("generic error"),
+			checkRan:               true,
+			expectedError:          true,
+		},
+		{
+			name: "should be okay if error is provisioned dashboard not found",
+			inputObj: &v0alpha1.Dashboard{
+				Spec: common.Unstructured{},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "Dashboard",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			deletionOptions: metav1.DeleteOptions{
+				GracePeriodSeconds: nil,
+			},
+			dashboardResponse:      nil,
+			dashboardErrorResponse: dashboards.ErrProvisionedDashboardNotFound,
+			checkRan:               true,
+			expectedError:          false,
+		},
+		{
+			name: "Should still run the check for delete if grace period is not 0",
+			inputObj: &v0alpha1.Dashboard{
+				Spec: common.Unstructured{},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "Dashboard",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			deletionOptions: metav1.DeleteOptions{
+				GracePeriodSeconds: &oneInt64,
+			},
+			dashboardResponse:      nil,
+			dashboardErrorResponse: nil,
+			checkRan:               true,
+			expectedError:          false,
+		},
+		{
+			name: "should not run the check for delete if grace period is set to 0",
+			inputObj: &v0alpha1.Dashboard{
+				Spec: common.Unstructured{},
+				TypeMeta: metav1.TypeMeta{
+					Kind: "Dashboard",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			deletionOptions: metav1.DeleteOptions{
+				GracePeriodSeconds: &zeroInt64,
+			},
+			dashboardResponse:      nil,
+			dashboardErrorResponse: nil,
+			checkRan:               false,
+			expectedError:          false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fakeService := &dashboards.FakeDashboardProvisioning{}
+			fakeService.On("GetProvisionedDashboardDataByDashboardUID", mock.Anything, mock.Anything, mock.Anything).Return(tt.dashboardResponse, tt.dashboardErrorResponse).Once()
+			b := &DashboardsAPIBuilder{
+				ProvisioningDashboardService: fakeService,
+			}
+			err := b.Validate(context.Background(), admission.NewAttributesRecord(
+				tt.inputObj,
+				nil,
+				v0alpha1.DashboardResourceInfo.GroupVersionKind(),
+				"stacks-123",
+				tt.inputObj.Name,
+				v0alpha1.DashboardResourceInfo.GroupVersionResource(),
+				"",
+				admission.Operation("DELETE"),
+				&tt.deletionOptions,
+				true,
+				&user.SignedInUser{},
+			), nil)
+
+			if tt.expectedError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			if tt.checkRan {
+				fakeService.AssertCalled(t, "GetProvisionedDashboardDataByDashboardUID", mock.Anything, mock.Anything, mock.Anything)
+			} else {
+				fakeService.AssertNotCalled(t, "GetProvisionedDashboardDataByDashboardUID", mock.Anything, mock.Anything, mock.Anything)
+			}
+		})
+	}
+}

pkg/registry/apis/dashboard/v0alpha1/register.go

@@ -44,6 +44,7 @@ var (
 
 // This is used just so wire has something unique to return
 type DashboardsAPIBuilder struct {
+	dashboard.DashboardsAPIBuilder
 	dashboardService dashboards.DashboardService
 	features         featuremgmt.FeatureToggles
 
@@ -59,6 +60,7 @@ type DashboardsAPIBuilder struct {
 func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	apiregistration builder.APIRegistrar,
 	dashboardService dashboards.DashboardService,
+	provisioningDashboardService dashboards.DashboardProvisioningService,
 	accessControl accesscontrol.AccessControl,
 	provisioning provisioning.ProvisioningService,
 	dashStore dashboards.Store,
@@ -72,7 +74,9 @@ func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	namespacer := request.GetNamespaceMapper(cfg)
 	builder := &DashboardsAPIBuilder{
 		log: log.New("grafana-apiserver.dashboards.v0alpha1"),
-
+		DashboardsAPIBuilder: dashboard.DashboardsAPIBuilder{
+			ProvisioningDashboardService: provisioningDashboardService,
+		},
 		dashboardService: dashboardService,
 		features:         features,
 		accessControl:    accessControl,

pkg/registry/apis/dashboard/v1alpha1/register.go

@@ -42,6 +42,7 @@ var (
 
 // This is used just so wire has something unique to return
 type DashboardsAPIBuilder struct {
+	dashboard.DashboardsAPIBuilder
 	dashboardService dashboards.DashboardService
 	features         featuremgmt.FeatureToggles
 
@@ -56,6 +57,7 @@ type DashboardsAPIBuilder struct {
 func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	apiregistration builder.APIRegistrar,
 	dashboardService dashboards.DashboardService,
+	provisioningDashboardService dashboards.DashboardProvisioningService,
 	accessControl accesscontrol.AccessControl,
 	provisioning provisioning.ProvisioningService,
 	dashStore dashboards.Store,
@@ -69,7 +71,9 @@ func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	namespacer := request.GetNamespaceMapper(cfg)
 	builder := &DashboardsAPIBuilder{
 		log: log.New("grafana-apiserver.dashboards.v1alpha1"),
-
+		DashboardsAPIBuilder: dashboard.DashboardsAPIBuilder{
+			ProvisioningDashboardService: provisioningDashboardService,
+		},
 		dashboardService: dashboardService,
 		features:         features,
 		accessControl:    accessControl,

pkg/registry/apis/dashboard/v2alpha1/register.go

@@ -42,6 +42,7 @@ var (
 
 // This is used just so wire has something unique to return
 type DashboardsAPIBuilder struct {
+	dashboard.DashboardsAPIBuilder
 	dashboardService dashboards.DashboardService
 	features         featuremgmt.FeatureToggles
 
@@ -56,6 +57,7 @@ type DashboardsAPIBuilder struct {
 func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	apiregistration builder.APIRegistrar,
 	dashboardService dashboards.DashboardService,
+	provisioningDashboardService dashboards.DashboardProvisioningService,
 	accessControl accesscontrol.AccessControl,
 	provisioning provisioning.ProvisioningService,
 	dashStore dashboards.Store,
@@ -70,6 +72,9 @@ func RegisterAPIService(cfg *setting.Cfg, features featuremgmt.FeatureToggles,
 	builder := &DashboardsAPIBuilder{
 		log: log.New("grafana-apiserver.dashboards.v2alpha1"),
 
+		DashboardsAPIBuilder: dashboard.DashboardsAPIBuilder{
+			ProvisioningDashboardService: provisioningDashboardService,
+		},
 		dashboardService: dashboardService,
 		features:         features,
 		accessControl:    accessControl,

pkg/services/dashboards/service/dashboard_service.go

@@ -492,16 +492,16 @@ func (dr *DashboardServiceImpl) SoftDeleteDashboard(ctx context.Context, orgID i
 		return fmt.Errorf("feature flag %s is not enabled", featuremgmt.FlagDashboardRestore)
 	}
 
+	if dr.features.IsEnabledGlobally(featuremgmt.FlagKubernetesCliDashboards) {
+		// deletes in unistore are soft deletes, so we can just delete in the same way
+		return dr.deleteDashboardThroughK8s(ctx, &dashboards.DeleteDashboardCommand{OrgID: orgID, UID: dashboardUID}, true)
+	}
+
 	provisionedData, _ := dr.GetProvisionedDashboardDataByDashboardUID(ctx, orgID, dashboardUID)
 	if provisionedData != nil && provisionedData.ID != 0 {
 		return dashboards.ErrDashboardCannotDeleteProvisionedDashboard
 	}
 
-	if dr.features.IsEnabledGlobally(featuremgmt.FlagKubernetesCliDashboards) {
-		// deletes in unistore are soft deletes, so we can just delete in the same way
-		return dr.deleteDashboardThroughK8s(ctx, &dashboards.DeleteDashboardCommand{OrgID: orgID, UID: dashboardUID})
-	}
-
 	return dr.dashboardStore.SoftDeleteDashboard(ctx, orgID, dashboardUID)
 }
 
@@ -533,6 +533,12 @@ func (dr *DashboardServiceImpl) deleteDashboard(ctx context.Context, dashboardId
 	ctx, span := tracer.Start(ctx, "dashboards.service.deleteDashboard")
 	defer span.End()
 
+	cmd := &dashboards.DeleteDashboardCommand{OrgID: orgId, ID: dashboardId, UID: dashboardUID}
+
+	if dr.features.IsEnabledGlobally(featuremgmt.FlagKubernetesCliDashboards) {
+		return dr.deleteDashboardThroughK8s(ctx, cmd, validateProvisionedDashboard)
+	}
+
 	if validateProvisionedDashboard {
 		provisionedData, err := dr.GetProvisionedDashboardDataByDashboardID(ctx, dashboardId)
 		if err != nil {
@@ -544,12 +550,6 @@ func (dr *DashboardServiceImpl) deleteDashboard(ctx context.Context, dashboardId
 		}
 	}
 
-	cmd := &dashboards.DeleteDashboardCommand{OrgID: orgId, ID: dashboardId, UID: dashboardUID}
-
-	if dr.features.IsEnabledGlobally(featuremgmt.FlagKubernetesCliDashboards) {
-		return dr.deleteDashboardThroughK8s(ctx, cmd)
-	}
-
 	return dr.dashboardStore.DeleteDashboard(ctx, cmd)
 }
 
@@ -1218,7 +1218,7 @@ func (dr *DashboardServiceImpl) deleteAllDashboardThroughK8s(ctx context.Context
 	return nil
 }
 
-func (dr *DashboardServiceImpl) deleteDashboardThroughK8s(ctx context.Context, cmd *dashboards.DeleteDashboardCommand) error {
+func (dr *DashboardServiceImpl) deleteDashboardThroughK8s(ctx context.Context, cmd *dashboards.DeleteDashboardCommand, validateProvisionedDashboard bool) error {
 	// create a new context - prevents issues when the request stems from the k8s api itself
 	// otherwise the context goes through the handlers twice and causes issues
 	newCtx, cancel, err := dr.getK8sContext(ctx)
@@ -1245,7 +1245,16 @@ func (dr *DashboardServiceImpl) deleteDashboardThroughK8s(ctx context.Context, c
 		cmd.UID = result.UID
 	}
 
-	err = client.Delete(newCtx, cmd.UID, v1.DeleteOptions{})
+	// use a grace period of 0 to indicate to skip the check of deleting provisioned dashboards
+	var gracePeriod *int64
+	if !validateProvisionedDashboard {
+		noGracePeriod := int64(0)
+		gracePeriod = &noGracePeriod
+	}
+
+	err = client.Delete(newCtx, cmd.UID, v1.DeleteOptions{
+		GracePeriodSeconds: gracePeriod,
+	})
 	if err != nil {
 		return err
 	}

pkg/services/dashboards/service/dashboard_service_test.go

@@ -656,19 +656,22 @@ func TestDeleteDashboard(t *testing.T) {
 	t.Run("Should use Kubernetes client if feature flags are enabled", func(t *testing.T) {
 		ctx, k8sClientMock, k8sResourceMock := setupK8sDashboardTests(service)
 		k8sClientMock.On("getClient", mock.Anything, int64(1)).Return(k8sResourceMock, true).Once()
-		fakeStore.On("GetProvisionedDataByDashboardID", mock.Anything, mock.Anything).Return(nil, nil).Once()
+		k8sResourceMock.On("Delete", mock.Anything, mock.Anything, metav1.DeleteOptions{
-		k8sResourceMock.On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+			GracePeriodSeconds: nil,
+		}, mock.Anything).Return(nil).Once()
 
 		err := service.DeleteDashboard(ctx, 1, "uid", 1)
 		require.NoError(t, err)
 		k8sClientMock.AssertExpectations(t)
 	})
 
-	t.Run("If UID is not passed in, it should retrieve that first", func(t *testing.T) {
+	t.Run("When UID is not passed in for provisioned dashboards, should retrieve that first and set grace period to zero", func(t *testing.T) {
 		ctx, k8sClientMock, k8sResourceMock := setupK8sDashboardTests(service)
+		zeroInt64 := int64(0)
 		k8sClientMock.On("getClient", mock.Anything, int64(1)).Return(k8sResourceMock, true).Once()
-		fakeStore.On("GetProvisionedDataByDashboardID", mock.Anything, mock.Anything).Return(nil, nil).Once()
+		k8sResourceMock.On("Delete", mock.Anything, mock.Anything, metav1.DeleteOptions{
-		k8sResourceMock.On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()
+			GracePeriodSeconds: &zeroInt64,
+		}, mock.Anything).Return(nil).Once()
 		k8sClientMock.searcher.On("Search", mock.Anything).Return(&resource.ResourceSearchResponse{
 			Results: &resource.ResourceTable{
 				Columns: []*resource.ResourceTableColumnDefinition{
@@ -694,7 +697,7 @@ func TestDeleteDashboard(t *testing.T) {
 			},
 			TotalHits: 1,
 		}, nil)
-		err := service.DeleteDashboard(ctx, 1, "", 1)
+		err := service.DeleteProvisionedDashboard(ctx, 1, 1)
 		require.NoError(t, err)
 		k8sClientMock.AssertExpectations(t)
 		k8sClientMock.searcher.AssertExpectations(t)

pkg/tests/apis/dashboard/dashboards_test.go

@@ -81,8 +81,11 @@ func runDashboardTest(t *testing.T, helper *apis.K8sTestHelper) {
 		require.Equal(t, obj.GetUID(), updated.GetUID())
 		require.Less(t, obj.GetResourceVersion(), updated.GetResourceVersion())
 
-		// Delete the object
+		// Delete the object, skipping the provisioned dashboard check
-		err = client.Resource.Delete(ctx, created, metav1.DeleteOptions{})
+		zeroInt64 := int64(0)
+		err = client.Resource.Delete(ctx, created, metav1.DeleteOptions{
+			GracePeriodSeconds: &zeroInt64,
+		})
 		require.NoError(t, err)
 
 		// Now it is not in the list