Secrets: garbage collection (#110247)

* clean up older secret versions

* start gargbage collection worker as background service

* make gen-go

* fix typo

* make update-workspace

* undo go mod changes

* undo go work sum changes

* Update pkg/registry/apis/secret/garbagecollectionworker/worker.go

Co-authored-by: Matheus Macabu <macabu@users.noreply.github.com>

* Update pkg/registry/apis/secret/garbagecollectionworker/worker.go

Co-authored-by: Matheus Macabu <macabu@users.noreply.github.com>

* default gc_worker_batch_size to 1 minute

* fix typo

* fix typo

* add test to ensure cleaning up secure values is idempotent

* make gen-go

* make update-workspace

* undo go.mod and .sum changes

* undo enterprise imports

---------

Co-authored-by: Matheus Macabu <macabu.matheus@gmail.com>
Co-authored-by: Matheus Macabu <macabu@users.noreply.github.com>
This commit is contained in:
Bruno
2025-09-02 11:11:01 -03:00
committed by GitHub
co-authored by Matheus Macabu Matheus Macabu
parent d5eb3e291a
commit f8cd7049e8
30 changed files with 1069 additions and 35 deletions
+14
View File
@@ -0,0 +1,14 @@
package clock
import "time"
type Clock struct {
}
func ProvideClock() *Clock {
return &Clock{}
}
func (c *Clock) Now() time.Time {
return time.Now()
}
@@ -0,0 +1,7 @@
package contracts
import "time"
type Clock interface {
Now() time.Time
}
@@ -37,6 +37,8 @@ type SecureValueMetadataStorage interface {
SetVersionToActive(ctx context.Context, namespace xkube.Namespace, name string, version int64) error
SetVersionToInactive(ctx context.Context, namespace xkube.Namespace, name string, version int64) error
SetExternalID(ctx context.Context, namespace xkube.Namespace, name string, version int64, externalID ExternalID) error
Delete(ctx context.Context, namespace xkube.Namespace, name string, version int64) error
LeaseInactiveSecureValues(ctx context.Context, maxBatchSize uint16) ([]secretv1beta1.SecureValue, error)
}
type SecureValueService interface {
@@ -0,0 +1,117 @@
package garbagecollectionworker
import (
"context"
"errors"
"fmt"
"sync"
"time"
"github.com/grafana/grafana-app-sdk/logging"
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/registry/apis/secret/xkube"
"github.com/grafana/grafana/pkg/setting"
"golang.org/x/sync/semaphore"
)
// Secure values have the `active` flag set to false on creation and deletion.
// The `active` flag is set to true when the creation process succeeds.
// The worker deletes secure values that are inactive because the creation process failed
// or because the secure value has been deleted.
type Worker struct {
Cfg *setting.Cfg
secureValueMetadataStorage contracts.SecureValueMetadataStorage
keeperMetadataStorage contracts.KeeperMetadataStorage
keeperService contracts.KeeperService
}
func ProvideWorker(
cfg *setting.Cfg,
secureValueMetadataStorage contracts.SecureValueMetadataStorage,
keeperMetadataStorage contracts.KeeperMetadataStorage,
keeperService contracts.KeeperService) *Worker {
return &Worker{
Cfg: cfg,
secureValueMetadataStorage: secureValueMetadataStorage,
keeperMetadataStorage: keeperMetadataStorage,
keeperService: keeperService}
}
func (w *Worker) Run(ctx context.Context) error {
if !w.Cfg.SecretsManagement.GCWorkerEnabled {
return nil
}
timer := time.NewTicker(w.Cfg.SecretsManagement.GCWorkerPollInterval)
defer timer.Stop()
for {
select {
case <-ctx.Done():
return ctx.Err()
case <-timer.C:
timeoutCtx, cancel := context.WithTimeout(context.Background(), w.Cfg.SecretsManagement.GCWorkerPerSecureValueCleanupTimeout)
if _, err := w.CleanupInactiveSecureValues(timeoutCtx); err != nil {
logging.FromContext(timeoutCtx).Error("cleaning up inactive secure values", "error", err)
}
cancel()
}
}
}
func (w *Worker) CleanupInactiveSecureValues(ctx context.Context) ([]secretv1beta1.SecureValue, error) {
secureValues, err := w.secureValueMetadataStorage.LeaseInactiveSecureValues(ctx, w.Cfg.SecretsManagement.GCWorkerMaxBatchSize)
if err != nil {
return nil, fmt.Errorf("fetching inactive secure values that need to be cleaned up: %w", err)
}
if len(secureValues) == 0 {
return nil, nil
}
errs := make([]error, len(secureValues))
sema := semaphore.NewWeighted(int64(w.Cfg.SecretsManagement.GCWorkerMaxConcurrentCleanups))
wg := &sync.WaitGroup{}
wg.Add(len(secureValues))
for i, sv := range secureValues {
if err := sema.Acquire(ctx, 1); err != nil {
return nil, fmt.Errorf("acquiring semaphore: %w", err)
}
go func(i int, sv *secretv1beta1.SecureValue) {
defer sema.Release(1)
defer wg.Done()
errs[i] = w.Cleanup(ctx, sv)
}(i, &sv)
}
wg.Wait()
return secureValues, errors.Join(errs...)
}
func (w *Worker) Cleanup(ctx context.Context, sv *secretv1beta1.SecureValue) error {
keeperCfg, err := w.keeperMetadataStorage.GetKeeperConfig(ctx, sv.Namespace, sv.Spec.Keeper, contracts.ReadOpts{ForUpdate: false})
if err != nil {
return fmt.Errorf("fetching keeper config: namespace=%+v keeperName=%+v %w", sv.Namespace, sv.Spec.Keeper, err)
}
keeper, err := w.keeperService.KeeperForConfig(keeperCfg)
if err != nil {
return fmt.Errorf("getting keeper for config: namespace=%+v keeperName=%+v %w", sv.Namespace, sv.Spec.Keeper, err)
}
// Keeper deletion is idempotent
if err := keeper.Delete(ctx, keeperCfg, sv.Namespace, sv.Name, sv.Status.Version); err != nil {
return fmt.Errorf("deleting secure value from keeper: %w", err)
}
// Metadata deletion is not idempotent but not found errors are ignored
if err := w.secureValueMetadataStorage.Delete(ctx, xkube.Namespace(sv.Namespace), sv.Name, sv.Status.Version); err != nil && !errors.Is(err, contracts.ErrSecureValueNotFound) {
return fmt.Errorf("deleting secure value from metadata storage: %w", err)
}
return nil
}
@@ -0,0 +1,247 @@
package garbagecollectionworker_test
import (
"fmt"
"slices"
"testing"
"time"
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/registry/apis/secret/testutils"
"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"
"k8s.io/utils/ptr"
"pgregory.net/rapid"
)
func TestBasic(t *testing.T) {
t.Parallel()
t.Run("when no secure values exist, there's no work to do", func(t *testing.T) {
t.Parallel()
sut := testutils.Setup(t)
ids, err := sut.GarbageCollectionWorker.CleanupInactiveSecureValues(t.Context())
require.NoError(t, err)
require.Empty(t, ids)
})
t.Run("inactive secure values are not deleted immediately because of the grace period", func(t *testing.T) {
t.Parallel()
sut := testutils.Setup(t)
sv1, err := sut.CreateSv(t.Context())
require.NoError(t, err)
_, err = sut.DeleteSv(t.Context(), sv1.Namespace, sv1.Name)
require.NoError(t, err)
// Try to fetch inactive secure values for deletion
svs, err := sut.SecureValueMetadataStorage.LeaseInactiveSecureValues(t.Context(), 10)
require.NoError(t, err)
require.Empty(t, svs)
})
t.Run("secure values are fetched for deletion and deleted from keeper", func(t *testing.T) {
sut := testutils.Setup(t)
sv, err := sut.CreateSv(t.Context())
require.NoError(t, err)
keeperCfg, err := sut.KeeperMetadataStorage.GetKeeperConfig(t.Context(), sv.Namespace, sv.Spec.Keeper, contracts.ReadOpts{ForUpdate: false})
require.NoError(t, err)
keeper, err := sut.KeeperService.KeeperForConfig(keeperCfg)
require.NoError(t, err)
// Get the secret value once to make sure it's reachable
exposedValue, err := keeper.Expose(t.Context(), keeperCfg, sv.Namespace, sv.Name, sv.Status.Version)
require.NoError(t, err)
require.NotEmpty(t, exposedValue.DangerouslyExposeAndConsumeValue())
_, err = sut.DeleteSv(t.Context(), sv.Namespace, sv.Name)
require.NoError(t, err)
// Advance time to wait for grace period
sut.Clock.AdvanceBy(10 * time.Minute)
svs, err := sut.GarbageCollectionWorker.CleanupInactiveSecureValues(t.Context())
require.NoError(t, err)
require.Equal(t, 1, len(svs))
require.Equal(t, sv.UID, svs[0].UID)
svs, err = sut.GarbageCollectionWorker.CleanupInactiveSecureValues(t.Context())
require.NoError(t, err)
require.Empty(t, svs)
// Try to get the secreet value again to make sure it's been deleted from the keeper
exposedValue, err = keeper.Expose(t.Context(), keeperCfg, sv.Namespace, sv.Name, sv.Status.Version)
require.ErrorIs(t, err, encryption.ErrEncryptedValueNotFound)
require.Empty(t, exposedValue)
})
t.Run("cleaning up secure values is idempotent", func(t *testing.T) {
t.Parallel()
sut := testutils.Setup(t)
sv, err := sut.CreateSv(t.Context())
require.NoError(t, err)
_, err = sut.DeleteSv(t.Context(), sv.Namespace, sv.Name)
require.NoError(t, err)
// Clean up the same secure value twice and ensure it succeeds
require.NoError(t, sut.GarbageCollectionWorker.Cleanup(t.Context(), sv))
require.NoError(t, sut.GarbageCollectionWorker.Cleanup(t.Context(), sv))
})
}
var (
decryptersGen = rapid.SampledFrom([]string{"svc1", "svc2", "svc3", "svc4", "svc5"})
nameGen = rapid.SampledFrom([]string{"n1", "n2", "n3", "n4", "n5"})
namespaceGen = rapid.SampledFrom([]string{"ns1", "ns2", "ns3", "ns4", "ns5"})
anySecureValueGen = rapid.Custom(func(t *rapid.T) *secretv1beta1.SecureValue {
return &secretv1beta1.SecureValue{
ObjectMeta: metav1.ObjectMeta{
Name: nameGen.Draw(t, "name"),
Namespace: namespaceGen.Draw(t, "ns"),
},
Spec: secretv1beta1.SecureValueSpec{
Description: rapid.SampledFrom([]string{"d1", "d2", "d3", "d4", "d5"}).Draw(t, "description"),
Value: ptr.To(secretv1beta1.NewExposedSecureValue(rapid.SampledFrom([]string{"v1", "v2", "v3", "v4", "v5"}).Draw(t, "value"))),
Decrypters: rapid.SliceOfDistinct(decryptersGen, func(v string) string { return v }).Draw(t, "decrypters"),
},
Status: secretv1beta1.SecureValueStatus{},
}
})
)
func TestProperty(t *testing.T) {
t.Parallel()
tt := t
rapid.Check(t, func(t *rapid.T) {
sut := testutils.Setup(tt)
model := newModel()
t.Repeat(map[string]func(*rapid.T){
"create": func(t *rapid.T) {
sv := anySecureValueGen.Draw(t, "sv")
svCopy := deepCopy(sv)
createdSv, err := sut.CreateSv(t.Context(), testutils.CreateSvWithSv(sv))
svCopy.UID = createdSv.UID
modelErr := model.create(sut.Clock.Now(), svCopy)
require.ErrorIs(t, err, modelErr)
},
"delete": func(t *rapid.T) {
if len(model.items) == 0 {
return
}
i := rapid.IntRange(0, len(model.items)-1).Draw(t, "index")
sv := model.items[i]
modelErr := model.delete(sv.Namespace, sv.Name)
_, err := sut.DeleteSv(t.Context(), sv.Namespace, sv.Name)
require.ErrorIs(t, err, modelErr)
},
"cleanup": func(t *rapid.T) {
// Taken from secureValueMetadataStorage.acquireLeases
minAge := 300 * time.Second
maxBatchSize := sut.GarbageCollectionWorker.Cfg.SecretsManagement.GCWorkerMaxBatchSize
modelDeleted, modelErr := model.cleanupInactiveSecureValues(sut.Clock.Now(), minAge, maxBatchSize)
deleted, err := sut.GarbageCollectionWorker.CleanupInactiveSecureValues(t.Context())
require.ErrorIs(t, err, modelErr)
require.Equal(t, len(modelDeleted), len(deleted), "model and impl deleted a different number of secure values")
seen := make(map[types.UID]bool, 0)
for _, v := range modelDeleted {
seen[v.UID] = true
}
for _, v := range deleted {
require.True(t, seen[v.UID], "impl deleted a secure value that the model did not")
}
},
"advanceTime": func(t *rapid.T) {
duration := time.Duration(rapid.IntRange(1, 10).Draw(t, "minutes")) * time.Minute
sut.Clock.AdvanceBy(duration)
},
})
})
}
type model struct {
items []*modelSecureValue
}
type modelSecureValue struct {
*secretv1beta1.SecureValue
active bool
created time.Time
}
func newModel() *model {
return &model{
items: make([]*modelSecureValue, 0),
}
}
func (m *model) create(now time.Time, sv *secretv1beta1.SecureValue) error {
for _, item := range m.items {
if item.active && item.Namespace == sv.Namespace && item.Name == sv.Name {
item.active = false
break
}
}
m.items = append(m.items, &modelSecureValue{SecureValue: sv, active: true, created: now})
return nil
}
func (m *model) delete(ns string, name string) error {
for _, sv := range m.items {
if sv.active && sv.Namespace == ns && sv.Name == name {
sv.active = false
return nil
}
}
return contracts.ErrSecureValueNotFound
}
func (m *model) cleanupInactiveSecureValues(now time.Time, minAge time.Duration, maxBatchSize uint16) ([]*modelSecureValue, error) {
// Using a slice to allow duplicates
toDelete := make([]*modelSecureValue, 0)
for _, sv := range m.items {
if len(toDelete) >= int(maxBatchSize) {
break
}
if !sv.active && now.Sub(sv.created) > minAge {
toDelete = append(toDelete, sv)
}
}
// PERF: The slices are always small
m.items = slices.DeleteFunc(m.items, func(v1 *modelSecureValue) bool {
return slices.ContainsFunc(toDelete, func(v2 *modelSecureValue) bool {
return v2.UID == v1.UID
})
})
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)
}
@@ -3,6 +3,7 @@ package testutils
import (
"context"
"testing"
"time"
"github.com/grafana/authlib/authn"
"github.com/grafana/authlib/types"
@@ -21,6 +22,7 @@ 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/garbagecollectionworker"
"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"
@@ -32,6 +34,7 @@ import (
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/storage/secret/database"
encryptionstorage "github.com/grafana/grafana/pkg/storage/secret/encryption"
"github.com/grafana/grafana/pkg/storage/secret/metadata"
"github.com/grafana/grafana/pkg/storage/secret/migrator"
)
@@ -71,7 +74,9 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut {
keeperMetadataStorage, err := metadata.ProvideKeeperMetadataStorage(database, tracer, nil)
require.NoError(t, err)
secureValueMetadataStorage, err := metadata.ProvideSecureValueMetadataStorage(database, tracer, nil)
clock := NewFakeClock()
secureValueMetadataStorage, err := metadata.ProvideSecureValueMetadataStorage(clock, database, tracer, nil)
require.NoError(t, err)
// Initialize access client + access control
@@ -84,8 +89,11 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut {
defaultKey := "SdlklWklckeLS"
cfg := setting.NewCfg()
cfg.SecretsManagement = setting.SecretsManagerSettings{
CurrentEncryptionProvider: "secret_key.v1",
ConfiguredKMSProviders: map[string]map[string]string{"secret_key.v1": {"secret_key": defaultKey}},
CurrentEncryptionProvider: "secret_key.v1",
ConfiguredKMSProviders: map[string]map[string]string{"secret_key.v1": {"secret_key": defaultKey}},
GCWorkerEnabled: false,
GCWorkerMaxBatchSize: 2,
GCWorkerMaxConcurrentCleanups: 2,
}
store, err := encryptionstorage.ProvideDataKeyStorage(database, tracer, nil)
require.NoError(t, err)
@@ -143,6 +151,12 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut {
consolidationService := service.ProvideConsolidationService(tracer, globalDataKeyStore, encryptedValueStorage, globalEncryptedValueStorage, encryptionManager)
garbageCollectionWorker := garbagecollectionworker.ProvideWorker(
cfg,
secureValueMetadataStorage,
keeperMetadataStorage,
keeperService)
return Sut{
SecureValueService: secureValueService,
SecureValueMetadataStorage: secureValueMetadataStorage,
@@ -156,6 +170,10 @@ func Setup(t *testing.T, opts ...func(*SetupConfig)) Sut {
ConsolidationService: consolidationService,
EncryptionManager: encryptionManager,
GlobalDataKeyStore: globalDataKeyStore,
GarbageCollectionWorker: garbageCollectionWorker,
Clock: clock,
KeeperService: keeperService,
KeeperMetadataStorage: keeperMetadataStorage,
}
}
@@ -172,6 +190,11 @@ type Sut struct {
ConsolidationService contracts.ConsolidationService
EncryptionManager contracts.EncryptionManager
GlobalDataKeyStore contracts.GlobalDataKeyStorage
GarbageCollectionWorker *garbagecollectionworker.Worker
// The fake clock passed to implementations to make testing easier
Clock *FakeClock
KeeperService contracts.KeeperService
KeeperMetadataStorage contracts.KeeperMetadataStorage
}
type CreateSvConfig struct {
@@ -327,3 +350,19 @@ func CreateX509TestDir(t *testing.T) TestCertPaths {
CA: caCertFile.Name(),
}
}
type FakeClock struct {
Current time.Time
}
func NewFakeClock() *FakeClock {
return &FakeClock{Current: time.Now()}
}
func (c *FakeClock) Now() time.Time {
return c.Current
}
func (c *FakeClock) AdvanceBy(duration time.Duration) {
c.Current = c.Current.Add(duration)
}