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

Secrets: Fix secure value creation timestamp changing when updating it (#114290)

Browse Source
This commit is contained in:
Matheus Macabu
2025-11-21 16:31:17 +01:00
committed by GitHub
parent 0d67442f1a
commit 5e949fc955
12 changed files with 104 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
go.mod
+1 -1
View File
@@ -139,7 +139,6 @@ require (
github.com/matttproud/golang_protobuf_extensions v1.0.4 // @grafana/alerting-backend
github.com/microsoft/go-mssqldb v1.9.2 // @grafana/partner-datasources
github.com/migueleliasweb/go-github-mock v1.1.0 // @grafana/grafana-git-ui-sync-team
github.com/mitchellh/copystructure v1.2.0 // @grafana/grafana-operator-experience-squad
github.com/mitchellh/mapstructure v1.5.1-0.20231216201459-8508981c8b6c //@grafana/identity-access-team
github.com/mocktools/go-smtp-mock/v2 v2.5.1 // @grafana/grafana-backend-group
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // @grafana/alerting-backend
@@ -515,6 +514,7 @@ require (
github.com/miekg/dns v1.1.63 // indirect
github.com/minio/asm2plan9s v0.0.0-20200509001527-cdd76441f9d8 // indirect
github.com/minio/c2goasm v0.0.0-20190812172519-36a3d3bbc4f3 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/go-homedir v1.1.0 // indirect
github.com/mitchellh/go-wordwrap v1.0.1 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
pkg/registry/apis/secret/garbagecollectionworker/worker_test.go
+14 -12
View File
@@ -1,7 +1,6 @@
package garbagecollectionworker_test
import (
"fmt"
"slices"
"testing"
"time"
@@ -11,7 +10,6 @@ import (
"github.com/grafana/grafana/pkg/registry/apis/secret/testutils"
"github.com/grafana/grafana/pkg/registry/apis/secret/xkube"
"github.com/grafana/grafana/pkg/storage/secret/encryption"
"github.com/mitchellh/copystructure"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
@@ -133,7 +131,7 @@ func TestProperty(t *testing.T) {
t.Repeat(map[string]func(*rapid.T){
"create": func(t *rapid.T) {
sv := anySecureValueGen.Draw(t, "sv")
svCopy := deepCopy(sv)
svCopy := sv.DeepCopy()
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(sv))
svCopy.UID = createdSv.UID
@@ -194,13 +192,15 @@ func newModel() *model {
}
func (m *model) create(now time.Time, sv *secretv1beta1.SecureValue) error {
created := now
for _, item := range m.items {
if item.active && item.Namespace == sv.Namespace && item.Name == sv.Name {
item.active = false
created = item.created
break
}
}
m.items = append(m.items, &modelSecureValue{SecureValue: sv, active: true, created: now})
m.items = append(m.items, &modelSecureValue{SecureValue: sv, active: true, created: created})
return nil
}
@@ -219,6 +219,16 @@ func (m *model) cleanupInactiveSecureValues(now time.Time, minAge time.Duration,
// Using a slice to allow duplicates
toDelete := make([]*modelSecureValue, 0)
// The implementation query sorts by created time ascending
slices.SortFunc(m.items, func(a, b *modelSecureValue) int {
if a.created.Before(b.created) {
return -1
} else if a.created.After(b.created) {
return 1
}
return 0
})
for _, sv := range m.items {
if len(toDelete) >= int(maxBatchSize) {
break
@@ -238,11 +248,3 @@ func (m *model) cleanupInactiveSecureValues(now time.Time, minAge time.Duration,
return toDelete, nil
}
func deepCopy[T any](sv T) T {
copied, err := copystructure.Copy(sv)
if err != nil {
panic(fmt.Sprintf("failed to copy secure value: %v", err))
}
return copied.(T)
}
pkg/storage/secret/metadata/data/secure_value_get_latest_version.sql → pkg/storage/secret/metadata/data/secure_value_get_latest_version_and_created_at.sql
+3 -1
View File
@@ -1,5 +1,7 @@
SELECT
{{ .Ident "version" }}
{{ .Ident "created" }},
{{ .Ident "version" }},
{{ .Ident "active" }}
FROM
{{ .Ident "secret_secure_value" }}
WHERE
pkg/storage/secret/metadata/query.go
+5 -5
View File
@@ -34,9 +34,9 @@ var (
sqlSecureValueLeaseInactive = mustTemplate("secure_value_lease_inactive.sql")
sqlSecureValueListByLeaseToken = mustTemplate("secure_value_list_by_lease_token.sql")
sqlGetLatestSecureValueVersion = mustTemplate("secure_value_get_latest_version.sql")
sqlSecureValueSetVersionToActive = mustTemplate("secure_value_set_version_to_active.sql")
sqlSecureValueSetVersionToInactive = mustTemplate("secure_value_set_version_to_inactive.sql")
sqlGetLatestSecureValueVersionAndCreatedAt = mustTemplate("secure_value_get_latest_version_and_created_at.sql")
sqlSecureValueSetVersionToActive = mustTemplate("secure_value_set_version_to_active.sql")
sqlSecureValueSetVersionToInactive = mustTemplate("secure_value_set_version_to_inactive.sql")
)
func mustTemplate(filename string) *template.Template {
@@ -171,13 +171,13 @@ func (r readSecureValue) Validate() error {
return nil // TODO
}
type getLatestSecureValueVersion struct {
type getLatestSecureValueVersionAndCreatedAt struct {
sqltemplate.SQLTemplate
Namespace string
Name string
}
func (r getLatestSecureValueVersion) Validate() error {
func (r getLatestSecureValueVersionAndCreatedAt) Validate() error {
return nil
}
pkg/storage/secret/metadata/query_test.go
+2 -2
View File
@@ -141,10 +141,10 @@ func TestSecureValueQueries(t *testing.T) {
mocks.CheckQuerySnapshots(t, mocks.TemplateTestSetup{
RootDir: "testdata",
Templates: map[*template.Template][]mocks.TemplateTestCase{
sqlGetLatestSecureValueVersion: {
sqlGetLatestSecureValueVersionAndCreatedAt: {
{
Name: "get latest secure value version",
Data: &getLatestSecureValueVersion{
Data: &getLatestSecureValueVersionAndCreatedAt{
SQLTemplate: mocks.NewTestingSQLTemplate(),
Name: "name",
Namespace: "ns",
pkg/storage/secret/metadata/secure_value_model.go
+3 -4
View File
@@ -122,17 +122,16 @@ func (sv *secureValueDB) toKubernetes() (*secretv1beta1.SecureValue, error) {
}
// toCreateRow maps a Kubernetes resource into a DB row for new resources being created/inserted.
func toCreateRow(now time.Time, keeper string, sv *secretv1beta1.SecureValue, actorUID string) (*secureValueDB, error) {
func toCreateRow(createdAt, updatedAt int64, keeper string, sv *secretv1beta1.SecureValue, actorUID string) (*secureValueDB, error) {
row, err := toRow(keeper, sv, "")
if err != nil {
return nil, fmt.Errorf("failed to convert SecureValue to secureValueDB: %w", err)
}
timestamp := now.UTC().Unix()
row.GUID = uuid.New().String()
row.Created = timestamp
row.Created = createdAt
row.CreatedBy = actorUID
row.Updated = timestamp
row.Updated = updatedAt
row.UpdatedBy = actorUID
return row, nil
pkg/storage/secret/metadata/secure_value_store.go
+40 -16
View File
@@ -85,14 +85,14 @@ func (s *secureValueMetadataStorage) Create(ctx context.Context, keeper string,
var row *secureValueDB
err := s.db.Transaction(ctx, func(ctx context.Context) error {
latestVersion, err := s.getLatestVersion(ctx, xkube.Namespace(sv.Namespace), sv.Name)
latest, err := s.getLatestVersionAndCreatedAt(ctx, xkube.Namespace(sv.Namespace), sv.Name)
if err != nil {
return fmt.Errorf("fetching latest secure value version: %w", err)
}
version := int64(1)
if latestVersion != nil {
version = *latestVersion + 1
if latest.version > 0 {
version = latest.version + 1
}
// Some other concurrent request may have created the version we're trying to create,
@@ -102,7 +102,15 @@ func (s *secureValueMetadataStorage) Create(ctx context.Context, keeper string,
for {
sv.Status.Version = version
row, err = toCreateRow(s.clock.Now(), keeper, sv, actorUID)
now := s.clock.Now().UTC().Unix()
createdAt := now
if latest.createdAt > 0 {
createdAt = latest.createdAt
}
updatedAt := now
row, err = toCreateRow(createdAt, updatedAt, keeper, sv, actorUID)
if err != nil {
return fmt.Errorf("to create row: %w", err)
}
@@ -153,44 +161,60 @@ func (s *secureValueMetadataStorage) Create(ctx context.Context, keeper string,
return createdSecureValue, nil
}
func (s *secureValueMetadataStorage) getLatestVersion(ctx context.Context, namespace xkube.Namespace, name string) (*int64, error) {
ctx, span := s.tracer.Start(ctx, "SecureValueMetadataStorage.getLatestVersion", trace.WithAttributes(
type versionAndCreatedAt struct {
createdAt int64
version int64
}
func (s *secureValueMetadataStorage) getLatestVersionAndCreatedAt(ctx context.Context, namespace xkube.Namespace, name string) (versionAndCreatedAt, error) {
ctx, span := s.tracer.Start(ctx, "SecureValueMetadataStorage.getLatestVersionAndCreatedAt", trace.WithAttributes(
attribute.String("name", name),
attribute.String("namespace", namespace.String()),
))
defer span.End()
req := getLatestSecureValueVersion{
req := getLatestSecureValueVersionAndCreatedAt{
SQLTemplate: sqltemplate.New(s.dialect),
Namespace: namespace.String(),
Name: name,
}
q, err := sqltemplate.Execute(sqlGetLatestSecureValueVersion, req)
q, err := sqltemplate.Execute(sqlGetLatestSecureValueVersionAndCreatedAt, req)
if err != nil {
return nil, fmt.Errorf("execute template %q: %w", sqlGetLatestSecureValueVersion.Name(), err)
return versionAndCreatedAt{}, fmt.Errorf("execute template %q: %w", sqlGetLatestSecureValueVersionAndCreatedAt.Name(), err)
}
rows, err := s.db.QueryContext(ctx, q, req.GetArgs()...)
if err != nil {
return nil, fmt.Errorf("fetching latest version for secure value: namespace=%+v name=%+v %w", namespace, name, err)
return versionAndCreatedAt{}, fmt.Errorf("fetching latest version for secure value: namespace=%+v name=%+v %w", namespace, name, err)
}
defer func() { _ = rows.Close() }()
if err := rows.Err(); err != nil {
return nil, fmt.Errorf("error executing query: %w", err)
return versionAndCreatedAt{}, fmt.Errorf("error executing query: %w", err)
}
if !rows.Next() {
return nil, nil
return versionAndCreatedAt{}, nil
}
var version int64
if err := rows.Scan(&version); err != nil {
return nil, fmt.Errorf("scanning version from returned rows: %w", err)
var (
createdAt int64
version int64
active bool
)
if err := rows.Scan(&createdAt, &version, &active); err != nil {
return versionAndCreatedAt{}, fmt.Errorf("scanning version from returned rows: %w", err)
}
return &version, nil
if !active {
createdAt = 0
}
return versionAndCreatedAt{
createdAt: createdAt,
version: version,
}, nil
}
func (s *secureValueMetadataStorage) readActiveVersion(ctx context.Context, namespace xkube.Namespace, name string, opts contracts.ReadOpts) (secureValueDB, error) {
pkg/storage/secret/metadata/secure_value_store_test.go
+2 -2
View File
@@ -199,8 +199,8 @@ func TestPropertySecureValueMetadataStorage(t *testing.T) {
t.Repeat(map[string]func(*rapid.T){
"create": func(t *rapid.T) {
sv := anySecureValueGen.Draw(t, "sv")
modelCreatedSv, modelErr := model.create(sut.Clock.Now(), deepCopy(sv))
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(deepCopy(sv)))
modelCreatedSv, modelErr := model.create(sut.Clock.Now(), sv.DeepCopy())
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(sv.DeepCopy()))
if err != nil || modelErr != nil {
require.ErrorIs(t, err, modelErr)
return
pkg/storage/secret/metadata/secure_value_test.go
+25 -26
View File
@@ -6,7 +6,6 @@ import (
"testing"
"time"
"github.com/mitchellh/copystructure"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
@@ -81,8 +80,15 @@ func (m *model) readActiveVersion(namespace, name string) *modelSecureValue {
func (m *model) create(now time.Time, sv *secretv1beta1.SecureValue) (*secretv1beta1.SecureValue, error) {
keeper := m.getActiveKeeper(sv.Namespace)
sv = deepCopy(sv)
modelSv := &modelSecureValue{SecureValue: sv, active: false, created: now}
sv = sv.DeepCopy()
// Preserve the original creation time if this secure value already exists
created := now
if sv := m.readActiveVersion(sv.Namespace, sv.Name); sv != nil {
created = sv.created
}
modelSv := &modelSecureValue{SecureValue: sv, active: false, created: created}
modelSv.Status.Version = m.getNewVersionNumber(modelSv.Namespace, modelSv.Name)
modelSv.Status.ExternalID = fmt.Sprintf("%d", modelSv.Status.Version)
modelSv.Status.Keeper = keeper.name
@@ -126,7 +132,7 @@ func (m *model) createKeeper(keeper *secretv1beta1.Keeper) (*secretv1beta1.Keepe
m.keepers = append(m.keepers, &modelKeeper{namespace: keeper.Namespace, name: keeper.Name})
return deepCopy(keeper), nil
return keeper.DeepCopy(), nil
}
func (m *model) setKeeperAsActive(namespace, keeperName string) error {
@@ -196,7 +202,7 @@ func (m *model) decrypt(decrypter, namespace, name string) (map[string]decrypt.D
v.active {
if slices.ContainsFunc(v.Spec.Decrypters, func(d string) bool { return d == decrypter }) {
return map[string]decrypt.DecryptResult{
name: decrypt.NewDecryptResultValue(deepCopy(v).Spec.Value),
name: decrypt.NewDecryptResultValue(v.DeepCopy().Spec.Value),
}, nil
}
@@ -328,14 +334,14 @@ func TestModel(t *testing.T) {
now := time.Now()
// Create a secure value
sv1, err := m.create(now, deepCopy(sv))
sv1, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
require.Equal(t, sv.Namespace, sv1.Namespace)
require.Equal(t, sv.Name, sv1.Name)
require.EqualValues(t, 1, sv1.Status.Version)
// Create a new version of a secure value
sv2, err := m.create(now, deepCopy(sv))
sv2, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
require.Equal(t, sv.Namespace, sv2.Namespace)
require.Equal(t, sv.Name, sv2.Name)
@@ -349,25 +355,25 @@ func TestModel(t *testing.T) {
now := time.Now()
sv1, err := m.create(now, deepCopy(sv))
sv1, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
// Create a new version of a secure value by updating it
sv2, _, err := m.update(now, deepCopy(sv1))
sv2, _, err := m.update(now, sv1.DeepCopy())
require.NoError(t, err)
require.Equal(t, sv.Namespace, sv2.Namespace)
require.Equal(t, sv.Name, sv2.Name)
require.EqualValues(t, 2, sv2.Status.Version)
// Try updating a secure value that doesn't exist without specifying a value for it
sv3 := deepCopy(sv2)
sv3 := sv2.DeepCopy()
sv3.Name = "i_dont_exist"
sv3.Spec.Value = nil
_, _, err = m.update(now, sv3)
require.ErrorIs(t, err, contracts.ErrSecureValueNotFound)
// Updating a value that doesn't exist creates a new version
sv4 := deepCopy(sv3)
sv4 := sv3.DeepCopy()
sv4.Name = "i_dont_exist"
sv4.Spec.Value = ptr.To(secretv1beta1.NewExposedSecureValue("sv4"))
_, _, err = m.update(now, sv4)
@@ -380,7 +386,7 @@ func TestModel(t *testing.T) {
m := newModel()
now := time.Now()
sv1, err := m.create(now, deepCopy(sv))
sv1, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
// Deleting a secure value
@@ -407,7 +413,7 @@ func TestModel(t *testing.T) {
require.Equal(t, 0, len(list.Items))
// Create a secure value
sv1, err := m.create(now, deepCopy(sv))
sv1, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
// 1 secure value exists and it should be returned
@@ -434,7 +440,7 @@ func TestModel(t *testing.T) {
// Create a secure value
secret := "v1"
sv1, err := m.create(now, deepCopy(sv))
sv1, err := m.create(now, sv.DeepCopy())
require.NoError(t, err)
// Decrypt the just created secure value
@@ -459,8 +465,9 @@ func TestStateMachine(t *testing.T) {
"create": func(t *rapid.T) {
sv := anySecureValueGen.Draw(t, "sv")
modelCreatedSv, modelErr := model.create(sut.Clock.Now(), deepCopy(sv))
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(deepCopy(sv)))
modelCreatedSv, modelErr := model.create(sut.Clock.Now(), sv.DeepCopy())
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(sv.DeepCopy()))
if err != nil || modelErr != nil {
require.ErrorIs(t, err, modelErr)
return
@@ -471,8 +478,8 @@ func TestStateMachine(t *testing.T) {
},
"update": func(t *rapid.T) {
sv := updateSecureValueGen.Draw(t, "sv")
modelCreatedSv, _, modelErr := model.update(sut.Clock.Now(), deepCopy(sv))
createdSv, err := sut.UpdateSv(t.Context(), deepCopy(sv))
modelCreatedSv, _, modelErr := model.update(sut.Clock.Now(), sv.DeepCopy())
createdSv, err := sut.UpdateSv(t.Context(), sv.DeepCopy())
if err != nil || modelErr != nil {
require.ErrorIs(t, err, modelErr)
return
@@ -656,11 +663,3 @@ func TestSecureValueServiceExampleBased(t *testing.T) {
require.Equal(t, newSv1.Spec.Description, updatedSv.Spec.Description)
})
}
func deepCopy[T any](sv T) T {
copied, err := copystructure.Copy(sv)
if err != nil {
panic(fmt.Sprintf("failed to copy secure value: %v", err))
}
return copied.(T)
}
pkg/storage/secret/metadata/testdata/mysql--secure_value_get_latest_version-get latest secure value version.sql → pkg/storage/secret/metadata/testdata/mysql--secure_value_get_latest_version_and_created_at-get latest secure value version.sql
Vendored
+3 -1
View File
@@ -1,5 +1,7 @@
SELECT
`version`
`created`,
`version`,
`active`
FROM
`secret_secure_value`
WHERE
pkg/storage/secret/metadata/testdata/postgres--secure_value_get_latest_version-get latest secure value version.sql → pkg/storage/secret/metadata/testdata/postgres--secure_value_get_latest_version_and_created_at-get latest secure value version.sql
Vendored
+3 -1
View File
@@ -1,5 +1,7 @@
SELECT
"version"
"created",
"version",
"active"
FROM
"secret_secure_value"
WHERE
pkg/storage/secret/metadata/testdata/sqlite--secure_value_get_latest_version-get latest secure value version.sql → pkg/storage/secret/metadata/testdata/sqlite--secure_value_get_latest_version_and_created_at-get latest secure value version.sql
Vendored
+3 -1
View File
@@ -1,5 +1,7 @@
SELECT
"version"
"created",
"version",
"active"
FROM
"secret_secure_value"
WHERE