diff --git a/controllers/hcpvaultsecretsapp_controller.go b/controllers/hcpvaultsecretsapp_controller.go index 5e61248aa..860b4b866 100644 --- a/controllers/hcpvaultsecretsapp_controller.go +++ b/controllers/hcpvaultsecretsapp_controller.go @@ -7,11 +7,13 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "net/http" "regexp" "strconv" "strings" + "sync" "time" httptransport "github.com/go-openapi/runtime/client" @@ -24,6 +26,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -32,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" "github.com/hashicorp/vault-secrets-operator/credentials" "github.com/hashicorp/vault-secrets-operator/credentials/hcp" @@ -88,6 +92,7 @@ type HCPVaultSecretsAppReconciler struct { referenceCache ResourceReferenceCache GlobalTransformationOptions *helpers.GlobalTransformationOptions BackOffRegistry *BackOffRegistry + once sync.Once SecretsClient client.Client } @@ -289,6 +294,80 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R }, nil } +func (r *HCPVaultSecretsAppReconciler) startOrphanedShadowSecretCleanup(ctx context.Context, cleanupOrphanedShadowSecretInterval time.Duration) error { + var err error + + r.once.Do(func() { + for { + select { + case <-ctx.Done(): + if ctx.Err() != nil { + err = ctx.Err() + } + return + // runs the cleanup process once every hour or as specified by the user + case <-time.After(cleanupOrphanedShadowSecretInterval): + r.cleanupOrphanedShadowSecrets(ctx) + } + } + }) + + return err +} + +func (r *HCPVaultSecretsAppReconciler) cleanupOrphanedShadowSecrets(ctx context.Context) { + logger := log.FromContext(ctx).WithName("cleanupOrphanedShadowSecrets") + var errs error + + namespaceLabelKey := hvsaLabelPrefix + "/namespace" + nameLabelKey := hvsaLabelPrefix + "/name" + + // filtering only for dynamic secrets, also checking if namespace and name labels are present + secrets := corev1.SecretList{} + if err := r.List(ctx, &secrets, client.InNamespace(common.OperatorNamespace), + client.MatchingLabels{"app.kubernetes.io/component": "hvs-dynamic-secret-cache"}, + client.HasLabels{namespaceLabelKey, nameLabelKey}); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to list shadow secrets: %w", err)) + } + + for _, secret := range secrets.Items { + namespace := secret.Labels[namespaceLabelKey] + name := secret.Labels[nameLabelKey] + objKey := types.NamespacedName{Namespace: namespace, Name: name} + + o := &secretsv1beta1.HCPVaultSecretsApp{} + + // get the HCPVaultSecretsApp instance that the shadow secret belongs to (if applicable) + // no errors are returned in the loop because this could block the deletion of other + // orphaned shadow secrets that are further along in the list + err := r.Get(ctx, objKey, o) + if err != nil && !apierrors.IsNotFound(err) { + errs = errors.Join(errs, fmt.Errorf("failed to get HCPVaultSecretsApp: %w", err)) + continue + } + + // if the HCPVaultSecretsApp has been deleted, and the shadow secret belongs to it, delete both + if o.GetDeletionTimestamp() != nil && o.GetUID() == types.UID(secret.Labels[helpers.LabelOwnerRefUID]) { + if err := r.handleDeletion(ctx, o); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to handle deletion of HCPVaultSecretsApp %s: %w", o.Spec.AppName, err)) + } + + logger.Info("Deleted orphaned resources associated with HCPVaultSecretsApp", "app", o.Name) + } else if apierrors.IsNotFound(err) || secret.GetDeletionTimestamp() != nil { + // otherwise, delete the single shadow secret if it has a deletion timestamp + if err := r.Delete(ctx, &secret); err != nil { + errs = errors.Join(errs, fmt.Errorf("failed to delete shadow secret %s: %w", secret.Name, err)) + } + + logger.Info("Deleted orphaned shadow secret", "secret", secret.Name) + } + } + + if errs != nil { + logger.Error(errs, "Failed during cleanup of orphaned shadow secrets") + } +} + func (r *HCPVaultSecretsAppReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.HCPVaultSecretsApp) error { o.Status.LastGeneration = o.GetGeneration() if err := r.Status().Update(ctx, o); err != nil { @@ -301,12 +380,17 @@ func (r *HCPVaultSecretsAppReconciler) updateStatus(ctx context.Context, o *secr } // SetupWithManager sets up the controller with the Manager. -func (r *HCPVaultSecretsAppReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options) error { +func (r *HCPVaultSecretsAppReconciler) SetupWithManager(mgr ctrl.Manager, opts controller.Options, cleanupShadowSecretInterval time.Duration) error { r.referenceCache = newResourceReferenceCache() if r.BackOffRegistry == nil { r.BackOffRegistry = NewBackOffRegistry() } + mgr.Add(manager.RunnableFunc(func(ctx context.Context) error { + err := r.startOrphanedShadowSecretCleanup(ctx, cleanupShadowSecretInterval) + return err + })) + return ctrl.NewControllerManagedBy(mgr). For(&secretsv1beta1.HCPVaultSecretsApp{}). WithEventFilter(syncableSecretPredicate(nil)). @@ -372,14 +456,18 @@ func (r *HCPVaultSecretsAppReconciler) hvsClient(ctx context.Context, o *secrets return hvsclient.New(cl, nil), nil } -func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o client.Object) error { +func (r *HCPVaultSecretsAppReconciler) handleDeletion(ctx context.Context, o *secretsv1beta1.HCPVaultSecretsApp) error { logger := log.FromContext(ctx) objKey := client.ObjectKeyFromObject(o) r.referenceCache.Remove(SecretTransformation, objKey) r.BackOffRegistry.Delete(objKey) - shadowObjKey := makeShadowObjKey(o) - if err := helpers.DeleteSecret(ctx, r.Client, shadowObjKey); err != nil { - logger.Error(err, "Failed to delete shadow secret", "shadow secret", shadowObjKey) + // delete all of the shadow secrets for this HCPVaultSecretsApp + err := r.DeleteAllOf(ctx, &corev1.Secret{}, + client.InNamespace(common.OperatorNamespace), + client.MatchingLabels{helpers.LabelOwnerRefUID: string(o.GetUID())}, + client.MatchingLabels{"app.kubernetes.io/component": "hvs-dynamic-secret-cache"}) + if err != nil { + return err } if controllerutil.ContainsFinalizer(o, hcpVaultSecretsAppFinalizer) { logger.Info("Removing finalizer") diff --git a/controllers/hcpvaultsecretsapp_controller_test.go b/controllers/hcpvaultsecretsapp_controller_test.go index e07f9d0cc..de78f56d6 100644 --- a/controllers/hcpvaultsecretsapp_controller_test.go +++ b/controllers/hcpvaultsecretsapp_controller_test.go @@ -18,12 +18,16 @@ import ( "github.com/hashicorp/hcp-sdk-go/clients/cloud-vault-secrets/preview/2023-11-28/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" secretsv1beta1 "github.com/hashicorp/vault-secrets-operator/api/v1beta1" "github.com/hashicorp/vault-secrets-operator/common" "github.com/hashicorp/vault-secrets-operator/helpers" + "github.com/hashicorp/vault-secrets-operator/internal/testutils" ) var _ runtime.ClientTransport = (*fakeHVSTransport)(nil) @@ -1224,3 +1228,155 @@ func Test_makeShadowObjKey(t *testing.T) { }) } } + +func Test_CleanupOrphanedShadowSecrets(t *testing.T) { + deletionTimestamp := metav1.Now() + + hvsApp := &secretsv1beta1.HCPVaultSecretsApp{ + TypeMeta: metav1.TypeMeta{ + Kind: HCPVaultSecretsApp.String(), + APIVersion: secretsv1beta1.GroupVersion.Version, + }, + ObjectMeta: metav1.ObjectMeta{ + UID: "hvsAppUID", + Namespace: "hvsAppNamespace", + Name: "hvsApp", + Finalizers: []string{hcpVaultSecretsAppFinalizer}, + }, + } + + tests := map[string]struct { + o *secretsv1beta1.HCPVaultSecretsApp + secret *corev1.Secret + isHCPVaultSecretsAppDeletionExpected bool + isShadowSecretDeletionExpected bool + }{ + "deleted-secret-hvsapp-owner": { + o: hvsApp.DeepCopy(), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: common.OperatorNamespace, + Name: "shadowSecret", + DeletionTimestamp: &deletionTimestamp, + Labels: map[string]string{ + hvsaLabelPrefix + "/namespace": hvsApp.GetNamespace(), + hvsaLabelPrefix + "/name": hvsApp.GetName(), + helpers.LabelOwnerRefUID: string(hvsApp.GetUID()), + "app.kubernetes.io/component": "hvs-dynamic-secret-cache", + }, + }, + }, + isHCPVaultSecretsAppDeletionExpected: true, + isShadowSecretDeletionExpected: true, + }, + "deleted-secret-hvsapp-not-owner": { + o: hvsApp.DeepCopy(), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: common.OperatorNamespace, + Name: "shadowSecret", + DeletionTimestamp: &deletionTimestamp, + Labels: map[string]string{ + hvsaLabelPrefix + "/namespace": "", + hvsaLabelPrefix + "/name": "", + helpers.LabelOwnerRefUID: "", + "app.kubernetes.io/component": "hvs-dynamic-secret-cache", + }, + }, + }, + isShadowSecretDeletionExpected: true, + }, + "deleted-secret-hvsapp-not-found": { + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: common.OperatorNamespace, + Name: "shadowSecret", + DeletionTimestamp: &deletionTimestamp, + Labels: map[string]string{ + hvsaLabelPrefix + "/namespace": "", + hvsaLabelPrefix + "/name": "", + helpers.LabelOwnerRefUID: "", + "app.kubernetes.io/component": "hvs-dynamic-secret-cache", + }, + }, + }, + isShadowSecretDeletionExpected: true, + }, + "secret-not-dynamic": { + o: hvsApp.DeepCopy(), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: common.OperatorNamespace, + Name: "nonShadowSecret", + DeletionTimestamp: &deletionTimestamp, + Labels: map[string]string{ + hvsaLabelPrefix + "/namespace": hvsApp.GetNamespace(), + hvsaLabelPrefix + "/name": hvsApp.GetName(), + helpers.LabelOwnerRefUID: string(hvsApp.GetUID()), + }, + }, + }, + isShadowSecretDeletionExpected: false, + }, + "non-deleted-secret": { + o: hvsApp.DeepCopy(), + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: common.OperatorNamespace, + Name: "nonShadowSecret", + Labels: map[string]string{ + hvsaLabelPrefix + "/namespace": hvsApp.GetNamespace(), + hvsaLabelPrefix + "/name": hvsApp.GetName(), + helpers.LabelOwnerRefUID: string(hvsApp.GetUID()), + "app.kubernetes.io/component": "hvs-dynamic-secret-cache", + }, + }, + }, + isShadowSecretDeletionExpected: false, + }, + } + + ctx := context.Background() + clientBuilder := testutils.NewFakeClientBuilder() + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + client := clientBuilder.Build() + r := &HCPVaultSecretsAppReconciler{ + Client: client, + BackOffRegistry: NewBackOffRegistry(), + referenceCache: newResourceReferenceCache(), + } + + // create the HCPVaultSecretsApp if the test case has one + if tt.o != nil { + assert.NoError(t, client.Create(ctx, tt.o)) + } + + // create the secret for the test case + assert.NoError(t, client.Create(ctx, tt.secret)) + + // DeleteTimestamp is a read-only field, so Delete will need to be called to + // simulate deletion of the HCPVaultSecretsApp + if tt.isHCPVaultSecretsAppDeletionExpected { + assert.NoError(t, client.Delete(ctx, tt.o)) + } + + r.cleanupOrphanedShadowSecrets(ctx) + + if tt.isHCPVaultSecretsAppDeletionExpected { + deletedHVSApp := &secretsv1beta1.HCPVaultSecretsApp{} + err := r.Get(ctx, ctrlclient.ObjectKeyFromObject(tt.o), deletedHVSApp) + assert.True(t, apierrors.IsNotFound(err)) + } + + secret := &corev1.Secret{} + err := r.Get(ctx, ctrlclient.ObjectKeyFromObject(tt.secret), secret) + if tt.isShadowSecretDeletionExpected { + assert.True(t, apierrors.IsNotFound(err)) + } else { + assert.False(t, apierrors.IsNotFound(err)) + } + }) + } +} diff --git a/helpers/secrets.go b/helpers/secrets.go index b578bb0ba..f18852bc0 100644 --- a/helpers/secrets.go +++ b/helpers/secrets.go @@ -45,7 +45,7 @@ var SecretDataErrorContainsRaw = fmt.Errorf("key '%s' not permitted in Secret da // labelOwnerRefUID is used as the primary key when listing the Secrets owned by // a specific VSO object. It should be included in every Secret that is created // by VSO. -var labelOwnerRefUID = fmt.Sprintf("%s/vso-ownerRefUID", secretsv1beta1.GroupVersion.Group) +var LabelOwnerRefUID = fmt.Sprintf("%s/vso-ownerRefUID", secretsv1beta1.GroupVersion.Group) // OwnerLabels will be applied to any k8s secret we create. They are used in Secret ownership checks. // There are similar labels in the vault package. It's important that component secret's value never @@ -71,7 +71,7 @@ func OwnerLabelsForObj(obj ctrlclient.Object) (map[string]string, error) { for k, v := range OwnerLabels { l[k] = v } - l[labelOwnerRefUID] = uid + l[LabelOwnerRefUID] = uid return l, nil } @@ -89,7 +89,7 @@ func matchingLabelsForObj(obj ctrlclient.Object) (ctrlclient.MatchingLabels, err for k, v := range l { m[k] = v } - m[labelOwnerRefUID] = string(obj.GetUID()) + m[LabelOwnerRefUID] = string(obj.GetUID()) return m, nil } diff --git a/helpers/secrets_test.go b/helpers/secrets_test.go index 2462f39fe..6e2fedfe2 100644 --- a/helpers/secrets_test.go +++ b/helpers/secrets_test.go @@ -52,7 +52,7 @@ func TestFindSecretsOwnedByObj(t *testing.T) { for k, v := range OwnerLabels { ownerLabels[k] = v } - ownerLabels[labelOwnerRefUID] = string(owner.GetUID()) + ownerLabels[LabelOwnerRefUID] = string(owner.GetUID()) notOwner := &secretsv1beta1.VaultDynamicSecret{ TypeMeta: metav1.TypeMeta{ @@ -489,7 +489,7 @@ func TestSyncSecret(t *testing.T) { } if tt.obj.GetUID() != "" { - ownerLabels[labelOwnerRefUID] = string(tt.obj.GetUID()) + ownerLabels[LabelOwnerRefUID] = string(tt.obj.GetUID()) } var orphans []ctrlclient.ObjectKey diff --git a/internal/options/env.go b/internal/options/env.go index 75456dd07..27c06d020 100644 --- a/internal/options/env.go +++ b/internal/options/env.go @@ -49,6 +49,9 @@ type VSOEnvOptions struct { // ClientCacheNumLocks is VSO_CLIENT_CACHE_NUM_LOCKS environment variable option ClientCacheNumLocks *int `split_words:"true"` + + // CleanupShadowSecretIntervalHVSA is VSO_CLEANUP_SHADOW_SECRET_INTERVAL_HVSA environment variable option + CleanupShadowSecretIntervalHVSA time.Duration `split_words:"true"` } // Parse environment variable options, prefixed with "VSO_" diff --git a/internal/options/env_test.go b/internal/options/env_test.go index e245c850f..e3e405076 100644 --- a/internal/options/env_test.go +++ b/internal/options/env_test.go @@ -23,32 +23,34 @@ func TestParse(t *testing.T) { }, "set all": { envs: map[string]string{ - "VSO_OUTPUT_FORMAT": "json", - "VSO_CLIENT_CACHE_SIZE": "100", - "VSO_CLIENT_CACHE_PERSISTENCE_MODEL": "memory", - "VSO_MAX_CONCURRENT_RECONCILES": "10", - "VSO_BACKOFF_INITIAL_INTERVAL": "1s", - "VSO_BACKOFF_MAX_INTERVAL": "60s", - "VSO_BACKOFF_MAX_ELAPSED_TIME": "24h", - "VSO_BACKOFF_RANDOMIZATION_FACTOR": "0.5", - "VSO_BACKOFF_MULTIPLIER": "2.5", - "VSO_GLOBAL_TRANSFORMATION_OPTIONS": "gOpt1,gOpt2", - "VSO_GLOBAL_VAULT_AUTH_OPTIONS": "vOpt1,vOpt2", - "VSO_CLIENT_CACHE_NUM_LOCKS": "10", + "VSO_OUTPUT_FORMAT": "json", + "VSO_CLIENT_CACHE_SIZE": "100", + "VSO_CLIENT_CACHE_PERSISTENCE_MODEL": "memory", + "VSO_MAX_CONCURRENT_RECONCILES": "10", + "VSO_BACKOFF_INITIAL_INTERVAL": "1s", + "VSO_BACKOFF_MAX_INTERVAL": "60s", + "VSO_BACKOFF_MAX_ELAPSED_TIME": "24h", + "VSO_BACKOFF_RANDOMIZATION_FACTOR": "0.5", + "VSO_BACKOFF_MULTIPLIER": "2.5", + "VSO_GLOBAL_TRANSFORMATION_OPTIONS": "gOpt1,gOpt2", + "VSO_GLOBAL_VAULT_AUTH_OPTIONS": "vOpt1,vOpt2", + "VSO_CLIENT_CACHE_NUM_LOCKS": "10", + "VSO_CLEANUP_SHADOW_SECRET_INTERVAL_HVSA": "1h", }, wantOptions: VSOEnvOptions{ - OutputFormat: "json", - ClientCacheSize: ptr.To(100), - ClientCachePersistenceModel: "memory", - MaxConcurrentReconciles: ptr.To(10), - BackoffInitialInterval: time.Second * 1, - BackoffMaxInterval: time.Second * 60, - BackoffMaxElapsedTime: time.Hour * 24, - BackoffRandomizationFactor: 0.5, - BackoffMultiplier: 2.5, - GlobalTransformationOptions: []string{"gOpt1", "gOpt2"}, - GlobalVaultAuthOptions: []string{"vOpt1", "vOpt2"}, - ClientCacheNumLocks: ptr.To(10), + OutputFormat: "json", + ClientCacheSize: ptr.To(100), + ClientCachePersistenceModel: "memory", + MaxConcurrentReconciles: ptr.To(10), + BackoffInitialInterval: time.Second * 1, + BackoffMaxInterval: time.Second * 60, + BackoffMaxElapsedTime: time.Hour * 24, + BackoffRandomizationFactor: 0.5, + BackoffMultiplier: 2.5, + GlobalTransformationOptions: []string{"gOpt1", "gOpt2"}, + GlobalVaultAuthOptions: []string{"vOpt1", "vOpt2"}, + ClientCacheNumLocks: ptr.To(10), + CleanupShadowSecretIntervalHVSA: time.Hour * 1, }, }, } diff --git a/main.go b/main.go index 2b8f5f8fb..8e9be42fd 100644 --- a/main.go +++ b/main.go @@ -147,6 +147,7 @@ func main() { var backoffRandomizationFactor float64 var backoffMultiplier float64 var backoffMaxElapsedTime time.Duration + var cleanupShadowSecretIntervalHVSA time.Duration // command-line args and flags flag.BoolVar(&printVersion, "version", false, "Print the operator version information") @@ -216,6 +217,10 @@ func main() { "All errors are tried using an exponential backoff strategy. "+ "The value must be greater than zero. "+ "Also set from environment variable VSO_BACKOFF_MULTIPLIER.") + flag.DurationVar(&cleanupShadowSecretIntervalHVSA, "cleanup-shadow-secret-interval-hvsa", 1*time.Hour, + "The time interval between each execution of the cleanup process for cached shadow secrets"+ + "associated with a deleted HCPVaultSecretsApp. "+ + "Also set from environment variable VSO_CLEANUP_SHADOW_SECRET_INTERVAL_HVSA.") opts := zap.Options{ Development: os.Getenv("VSO_LOGGER_DEVELOPMENT_MODE") != "", @@ -264,6 +269,9 @@ func main() { if vsoEnvOptions.BackoffMultiplier != 0 { backoffMultiplier = vsoEnvOptions.BackoffMultiplier } + if vsoEnvOptions.CleanupShadowSecretIntervalHVSA != 0 { + cleanupShadowSecretIntervalHVSA = vsoEnvOptions.CleanupShadowSecretIntervalHVSA + } if len(vsoEnvOptions.GlobalVaultAuthOptions) > 0 { globalVaultAuthOptsSet = vsoEnvOptions.GlobalVaultAuthOptions } else if globalVaultAuthOpts != "" { @@ -590,7 +598,7 @@ func main() { MinRefreshAfter: minRefreshAfterHVSA, BackOffRegistry: controllers.NewBackOffRegistry(backoffOpts...), GlobalTransformationOptions: globalTransOptions, - }).SetupWithManager(mgr, controllerOptions); err != nil { + }).SetupWithManager(mgr, controllerOptions, cleanupShadowSecretIntervalHVSA); err != nil { setupLog.Error(err, "unable to create controller", "controller", "HCPVaultSecretsApp") os.Exit(1) }