SecureValues: Run mutate and validation hooks in service layer (#109379)

* SecureValues: Run mutate and validation hooks in service layer

* add some unit tests
This commit is contained in:
Matheus Macabu
2025-08-08 13:15:23 +02:00
committed by GitHub
parent 71b6149492
commit 01c1a6ce5b
12 changed files with 418 additions and 20 deletions
@@ -0,0 +1,14 @@
package contracts
import (
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
"k8s.io/apiserver/pkg/admission"
)
type SecureValueMutator interface {
Mutate(sv *secretv1beta1.SecureValue, operation admission.Operation) error
}
type KeeperMutator interface {
Mutate(kp *secretv1beta1.Keeper, operation admission.Operation) error
}
@@ -5,6 +5,7 @@ import (
"k8s.io/apiserver/pkg/admission"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
"github.com/grafana/grafana/pkg/registry/apis/secret/xkube"
)
type SecureValueValidator interface {
@@ -14,3 +15,21 @@ type SecureValueValidator interface {
type KeeperValidator interface {
Validate(keeper *secretv1beta1.Keeper, oldKeeper *secretv1beta1.Keeper, operation admission.Operation) field.ErrorList
}
type ErrValidateSecureValue struct {
list field.ErrorList
}
var _ xkube.ErrorLister = (*ErrValidateSecureValue)(nil)
func NewErrValidateSecureValue(list field.ErrorList) *ErrValidateSecureValue {
return &ErrValidateSecureValue{list: list}
}
func (e *ErrValidateSecureValue) Error() string {
return e.list.ToAggregate().Error()
}
func (e *ErrValidateSecureValue) ErrorList() field.ErrorList {
return e.list
}
@@ -17,7 +17,6 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/registry/apis/secret/contracts"
"github.com/grafana/grafana/pkg/registry/apis/secret/xkube"
"github.com/grafana/grafana/pkg/util"
)
type LocalInlineSecureValueService struct {
@@ -209,8 +208,7 @@ func (s *LocalInlineSecureValueService) CreateInline(ctx context.Context, owner
obj := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
//GenerateName: "inline-",
Name: "inline-" + util.GenerateShortUID(),
GenerateName: "inline-",
Namespace: owner.Namespace,
OwnerReferences: []metav1.OwnerReference{owner.ToOwnerReference()},
},
@@ -223,7 +221,7 @@ func (s *LocalInlineSecureValueService) CreateInline(ctx context.Context, owner
createdSv, err := s.secureValueService.Create(ctx, obj, authInfo.GetUID())
if err != nil {
return "", fmt.Errorf("error creating secure value %s for owner %v: %w", obj.Name, owner, err)
return "", fmt.Errorf("error creating secure value for owner %v: %w", owner, err)
}
return createdSv.GetName(), nil
@@ -0,0 +1,31 @@
package mutator
import (
"cmp"
"fmt"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
"github.com/grafana/grafana/pkg/registry/apis/secret/contracts"
"github.com/grafana/grafana/pkg/util"
"k8s.io/apiserver/pkg/admission"
)
type keeperMutator struct{}
var _ contracts.KeeperMutator = &keeperMutator{}
func ProvideKeeperMutator() contracts.KeeperMutator {
return &keeperMutator{}
}
func (*keeperMutator) Mutate(kp *secretv1beta1.Keeper, operation admission.Operation) error {
if kp == nil {
return fmt.Errorf("expected Keeper to be non-nil")
}
if operation == admission.Create && kp.Name == "" {
kp.SetName(cmp.Or(kp.GetGenerateName(), "kp-") + util.GenerateShortUID())
}
return nil
}
@@ -0,0 +1,71 @@
package mutator
import (
"strings"
"testing"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apiserver/pkg/admission"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
)
func TestKeeperMutator(t *testing.T) {
mutator := ProvideKeeperMutator()
t.Run("when keeper is nil, it returns an error", func(t *testing.T) {
err := mutator.Mutate(nil, admission.Create)
require.Error(t, err)
require.Equal(t, "expected Keeper to be non-nil", err.Error())
})
t.Run("when operation is Create and name is empty without GenerateName", func(t *testing.T) {
keeper := &secretv1beta1.Keeper{}
err := mutator.Mutate(keeper, admission.Create)
require.NoError(t, err)
require.NotEmpty(t, keeper.Name)
require.True(t, strings.HasPrefix(keeper.Name, "kp-"))
})
t.Run("when operation is Create and name is already set", func(t *testing.T) {
keeper := &secretv1beta1.Keeper{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
}
originalName := keeper.Name
err := mutator.Mutate(keeper, admission.Create)
require.NoError(t, err)
require.Equal(t, originalName, keeper.Name)
})
t.Run("when operation is Create and GenerateName is set", func(t *testing.T) {
keeper := &secretv1beta1.Keeper{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "custom-prefix-",
},
}
err := mutator.Mutate(keeper, admission.Create)
require.NoError(t, err)
require.NotEmpty(t, keeper.Name)
require.True(t, strings.HasPrefix(keeper.Name, "custom-prefix-"))
})
t.Run("when operation is Create and both name and GenerateName are set", func(t *testing.T) {
keeper := &secretv1beta1.Keeper{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
GenerateName: "custom-prefix-",
},
}
originalName := keeper.Name
err := mutator.Mutate(keeper, admission.Create)
require.NoError(t, err)
require.Equal(t, originalName, keeper.Name)
})
}
@@ -0,0 +1,37 @@
package mutator
import (
"cmp"
"fmt"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
"github.com/grafana/grafana/pkg/registry/apis/secret/contracts"
"github.com/grafana/grafana/pkg/util"
"k8s.io/apiserver/pkg/admission"
)
type secureValueMutator struct{}
var _ contracts.SecureValueMutator = &secureValueMutator{}
func ProvideSecureValueMutator() contracts.SecureValueMutator {
return &secureValueMutator{}
}
func (*secureValueMutator) Mutate(sv *secretv1beta1.SecureValue, operation admission.Operation) error {
if sv == nil {
return fmt.Errorf("expected SecureValue to be non-nil")
}
if operation == admission.Create && sv.Name == "" {
sv.SetName(cmp.Or(sv.GetGenerateName(), "sv-") + util.GenerateShortUID())
}
// On any mutation to a `SecureValue`, clear the status.
if operation == admission.Create || operation == admission.Update {
sv.Status.ExternalID = ""
sv.Status.Version = 0
}
return nil
}
@@ -0,0 +1,179 @@
package mutator
import (
"strings"
"testing"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apiserver/pkg/admission"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
)
func TestSecureValueMutator(t *testing.T) {
mutator := ProvideSecureValueMutator()
populatedStatus := secretv1beta1.SecureValueStatus{
ExternalID: "existing-external-id",
Version: 1,
}
t.Run("when secure value is nil, it returns an error", func(t *testing.T) {
err := mutator.Mutate(nil, admission.Create)
require.Error(t, err)
require.Equal(t, "expected SecureValue to be non-nil", err.Error())
})
t.Run("when operation is Create and name is empty", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
Status: populatedStatus,
}
err := mutator.Mutate(secureValue, admission.Create)
require.NoError(t, err)
require.NotEmpty(t, secureValue.Name)
require.True(t, strings.HasPrefix(secureValue.Name, "sv-"))
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Create and name is already set", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
Status: populatedStatus,
}
originalName := secureValue.Name
err := mutator.Mutate(secureValue, admission.Create)
require.NoError(t, err)
require.Equal(t, originalName, secureValue.Name)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Create and GenerateName is set", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "custom-prefix-",
},
Status: populatedStatus,
}
err := mutator.Mutate(secureValue, admission.Create)
require.NoError(t, err)
require.NotEmpty(t, secureValue.Name)
require.True(t, strings.HasPrefix(secureValue.Name, "custom-prefix-"))
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Create and both name and GenerateName are set", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
GenerateName: "custom-prefix-",
},
Status: populatedStatus,
}
originalName := secureValue.Name
err := mutator.Mutate(secureValue, admission.Create)
require.NoError(t, err)
require.Equal(t, originalName, secureValue.Name)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Update", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
Status: populatedStatus,
}
originalName := secureValue.Name
err := mutator.Mutate(secureValue, admission.Update)
require.NoError(t, err)
require.Equal(t, originalName, secureValue.Name)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Update and name is empty", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
Status: populatedStatus,
}
err := mutator.Mutate(secureValue, admission.Update)
require.NoError(t, err)
require.Empty(t, secureValue.Name)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Delete", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
Status: populatedStatus,
}
originalName := secureValue.Name
originalExternalID := secureValue.Status.ExternalID
originalVersion := secureValue.Status.Version
err := mutator.Mutate(secureValue, admission.Delete)
require.NoError(t, err)
require.Equal(t, originalName, secureValue.Name)
require.Equal(t, originalExternalID, secureValue.Status.ExternalID)
require.Equal(t, originalVersion, secureValue.Status.Version)
})
t.Run("when operation is Connect", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
Status: populatedStatus,
}
originalName := secureValue.Name
originalExternalID := secureValue.Status.ExternalID
originalVersion := secureValue.Status.Version
err := mutator.Mutate(secureValue, admission.Connect)
require.NoError(t, err)
require.Equal(t, originalName, secureValue.Name)
require.Equal(t, originalExternalID, secureValue.Status.ExternalID)
require.Equal(t, originalVersion, secureValue.Status.Version)
})
t.Run("when operation is Create with empty status", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
}
err := mutator.Mutate(secureValue, admission.Create)
require.NoError(t, err)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
t.Run("when operation is Update with empty status", func(t *testing.T) {
secureValue := &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-name",
},
}
err := mutator.Mutate(secureValue, admission.Update)
require.NoError(t, err)
require.Empty(t, secureValue.Status.ExternalID)
require.Equal(t, int64(0), secureValue.Status.Version)
})
}
@@ -81,8 +81,9 @@ func TestConsolidation(t *testing.T) {
Namespace: tc.namespace,
},
Spec: secretv1beta1.SecureValueSpec{
Value: ptr.To(secretv1beta1.NewExposedSecureValue(tc.value)),
Decrypters: []string{"decrypter1"},
Description: "test description",
Value: ptr.To(secretv1beta1.NewExposedSecureValue(tc.value)),
Decrypters: []string{"decrypter1"},
},
}
@@ -158,8 +159,9 @@ func TestConsolidation(t *testing.T) {
Namespace: tc.namespace,
},
Spec: secretv1beta1.SecureValueSpec{
Value: ptr.To(secretv1beta1.NewExposedSecureValue(tc.value)),
Decrypters: []string{"decrypter1"},
Description: "test description",
Value: ptr.To(secretv1beta1.NewExposedSecureValue(tc.value)),
Decrypters: []string{"decrypter1"},
},
}
@@ -9,6 +9,7 @@ import (
claims "github.com/grafana/authlib/types"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
"k8s.io/apiserver/pkg/admission"
"github.com/grafana/grafana-app-sdk/logging"
secretv1beta1 "github.com/grafana/grafana/apps/secret/pkg/apis/secret/v1beta1"
@@ -27,6 +28,8 @@ type SecureValueService struct {
accessClient claims.AccessClient
database contracts.Database
secureValueMetadataStorage contracts.SecureValueMetadataStorage
secureValueValidator contracts.SecureValueValidator
secureValueMutator contracts.SecureValueMutator
keeperMetadataStorage contracts.KeeperMetadataStorage
keeperService contracts.KeeperService
metrics *metrics.SecureValueServiceMetrics
@@ -37,6 +40,8 @@ func ProvideSecureValueService(
accessClient claims.AccessClient,
database contracts.Database,
secureValueMetadataStorage contracts.SecureValueMetadataStorage,
secureValueValidator contracts.SecureValueValidator,
secureValueMutator contracts.SecureValueMutator,
keeperMetadataStorage contracts.KeeperMetadataStorage,
keeperService contracts.KeeperService,
reg prometheus.Registerer,
@@ -46,29 +51,34 @@ func ProvideSecureValueService(
accessClient: accessClient,
database: database,
secureValueMetadataStorage: secureValueMetadataStorage,
secureValueValidator: secureValueValidator,
secureValueMutator: secureValueMutator,
keeperMetadataStorage: keeperMetadataStorage,
keeperService: keeperService,
metrics: metrics.NewSecureValueServiceMetrics(reg),
}
}
func (s *SecureValueService) Create(ctx context.Context, sv *secretv1beta1.SecureValue, actorUID string) (_ *secretv1beta1.SecureValue, createErr error) {
func (s *SecureValueService) Create(ctx context.Context, sv *secretv1beta1.SecureValue, actorUID string) (createdSv *secretv1beta1.SecureValue, createErr error) {
start := time.Now()
name, namespace := sv.GetName(), sv.GetNamespace()
ctx, span := s.tracer.Start(ctx, "SecureValueService.Create", trace.WithAttributes(
attribute.String("name", name),
attribute.String("namespace", namespace),
attribute.String("namespace", sv.GetNamespace()),
attribute.String("actor", actorUID),
))
defer span.End()
defer func() {
args := []any{
"name", name,
"namespace", namespace,
"namespace", sv.GetNamespace(),
"actorUID", actorUID,
}
if createdSv != nil {
args = append(args, "name", createdSv.GetName())
span.SetAttributes(attribute.String("name", createdSv.GetName()))
}
success := createErr == nil
args = append(args, "success", success)
if !success {
@@ -151,6 +161,14 @@ func (s *SecureValueService) Update(ctx context.Context, newSecureValue *secretv
}
func (s *SecureValueService) createNewVersion(ctx context.Context, sv *secretv1beta1.SecureValue, actorUID string) (*secretv1beta1.SecureValue, error) {
if err := s.secureValueMutator.Mutate(sv, admission.Create); err != nil {
return nil, err
}
if errorList := s.secureValueValidator.Validate(sv, nil, admission.Create); len(errorList) > 0 {
return nil, contracts.NewErrValidateSecureValue(errorList)
}
createdSv, err := s.secureValueMetadataStorage.Create(ctx, sv, actorUID)
if err != nil {
return nil, fmt.Errorf("creating secure value: %w", err)
@@ -196,6 +214,13 @@ func (s *SecureValueService) createNewVersion(ctx context.Context, sv *secretv1b
}
func (s *SecureValueService) Read(ctx context.Context, namespace xkube.Namespace, name string) (_ *secretv1beta1.SecureValue, readErr error) {
if namespace == "" {
return nil, fmt.Errorf("namespace cannot be empty")
}
if name == "" {
return nil, fmt.Errorf("name cannot be empty")
}
start := time.Now()
ctx, span := s.tracer.Start(ctx, "SecureValueService.Read", trace.WithAttributes(
@@ -229,6 +254,10 @@ func (s *SecureValueService) Read(ctx context.Context, namespace xkube.Namespace
}
func (s *SecureValueService) List(ctx context.Context, namespace xkube.Namespace) (_ *secretv1beta1.SecureValueList, listErr error) {
if namespace == "" {
return nil, fmt.Errorf("namespace cannot be empty")
}
start := time.Now()
ctx, span := s.tracer.Start(ctx, "SecureValueService.List", trace.WithAttributes(
@@ -292,6 +321,13 @@ func (s *SecureValueService) List(ctx context.Context, namespace xkube.Namespace
}
func (s *SecureValueService) Delete(ctx context.Context, namespace xkube.Namespace, name string) (_ *secretv1beta1.SecureValue, deleteErr error) {
if namespace == "" {
return nil, fmt.Errorf("namespace cannot be empty")
}
if name == "" {
return nil, fmt.Errorf("name cannot be empty")
}
start := time.Now()
ctx, span := s.tracer.Start(ctx, "SecureValueService.Delete", trace.WithAttributes(
@@ -20,8 +20,10 @@ import (
cipher "github.com/grafana/grafana/pkg/registry/apis/secret/encryption/cipher/service"
osskmsproviders "github.com/grafana/grafana/pkg/registry/apis/secret/encryption/kmsproviders"
"github.com/grafana/grafana/pkg/registry/apis/secret/encryption/manager"
"github.com/grafana/grafana/pkg/registry/apis/secret/mutator"
"github.com/grafana/grafana/pkg/registry/apis/secret/secretkeeper/sqlkeeper"
"github.com/grafana/grafana/pkg/registry/apis/secret/service"
"github.com/grafana/grafana/pkg/registry/apis/secret/validator"
"github.com/grafana/grafana/pkg/registry/apis/secret/xkube"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
@@ -123,7 +125,10 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut {
keeperService = setupCfg.KeeperService
}
secureValueService := service.ProvideSecureValueService(tracer, accessClient, database, secureValueMetadataStorage, keeperMetadataStorage, keeperService, nil)
secureValueValidator := validator.ProvideSecureValueValidator()
secureValueMutator := mutator.ProvideSecureValueMutator()
secureValueService := service.ProvideSecureValueService(tracer, accessClient, database, secureValueMetadataStorage, secureValueValidator, secureValueMutator, keeperMetadataStorage, keeperService, nil)
decryptAuthorizer := decrypt.ProvideDecryptAuthorizer(tracer)
+3
View File
@@ -45,6 +45,7 @@ import (
cipher "github.com/grafana/grafana/pkg/registry/apis/secret/encryption/cipher/service"
encryptionManager "github.com/grafana/grafana/pkg/registry/apis/secret/encryption/manager"
secretinline "github.com/grafana/grafana/pkg/registry/apis/secret/inline"
secretmutator "github.com/grafana/grafana/pkg/registry/apis/secret/mutator"
secretsecurevalueservice "github.com/grafana/grafana/pkg/registry/apis/secret/service"
secretvalidator "github.com/grafana/grafana/pkg/registry/apis/secret/validator"
appregistry "github.com/grafana/grafana/pkg/registry/apps"
@@ -437,6 +438,8 @@ var wireBasicSet = wire.NewSet(
secretsecurevalueservice.ProvideSecureValueService,
secretvalidator.ProvideKeeperValidator,
secretvalidator.ProvideSecureValueValidator,
secretmutator.ProvideKeeperMutator,
secretmutator.ProvideSecureValueMutator,
secretmigrator.NewWithEngine,
secretdatabase.ProvideDatabase,
wire.Bind(new(secretcontracts.Database), new(*secretdatabase.Database)),
File diff suppressed because one or more lines are too long