Periodically Cleanup Orphaned Shadow Secrets

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
 }
 
 // +kubebuilder:rbac:groups=secrets.hashicorp.com,resources=hcpvaultsecretsapps,verbs=get;list;watch;create;update;patch;delete
@@ -288,6 +293,78 @@ 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 := helpers.DeleteSecret(ctx, r.Client, objKey); 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)
+		}
+	}
+
+	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 {
@@ -300,12 +377,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, cleanupOrphanedShadowSecretInterval 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, cleanupOrphanedShadowSecretInterval)
+		return err
+	}))
+
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&secretsv1beta1.HCPVaultSecretsApp{}).
 		WithEventFilter(syncableSecretPredicate(nil)).
@@ -371,7 +453,7 @@ 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)

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,159 @@ func Test_makeShadowObjKey(t *testing.T) {
 		})
 	}
 }
+
+func Test_CleanupOrphanedShadowSecrets(t *testing.T) {
+	deletionTimestamp := metav1.Now()
+
+	tests := map[string]struct {
+		o                                    *secretsv1beta1.HCPVaultSecretsApp
+		secret                               *corev1.Secret
+		isHCPVaultSecretsAppDeletionExpected bool
+		isShadowSecretDeletionExpected       bool
+	}{
+		"deleted-secret-hvsapp-owner": {
+			o: &secretsv1beta1.HCPVaultSecretsApp{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       HCPVaultSecretsApp.String(),
+					APIVersion: secretsv1beta1.GroupVersion.Version,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					UID:        "hvsApp1UID",
+					Namespace:  "hvsApp1Namespace",
+					Name:       "hvsApp1",
+					Finalizers: []string{hcpVaultSecretsAppFinalizer},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:         common.OperatorNamespace,
+					Name:              "shadowSecret1",
+					DeletionTimestamp: &deletionTimestamp,
+					Finalizers:        []string{vaultDynamicSecretFinalizer},
+					Labels: map[string]string{
+						hvsaLabelPrefix + "/namespace": "hvsApp1Namespace",
+						hvsaLabelPrefix + "/name":      "hvsApp1",
+						"app.kubernetes.io/component":  "hvs-dynamic-secret-cache",
+						helpers.LabelOwnerRefUID:       "hvsApp1UID",
+					},
+				},
+			},
+			isHCPVaultSecretsAppDeletionExpected: true,
+			isShadowSecretDeletionExpected:       true,
+		},
+		"deleted-secret-hvsapp-not-owner": {
+			o: &secretsv1beta1.HCPVaultSecretsApp{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       HCPVaultSecretsApp.String(),
+					APIVersion: secretsv1beta1.GroupVersion.Version,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					UID:        "hvsApp2UID",
+					Namespace:  "hvsApp2Namespace",
+					Name:       "hvsApp2",
+					Finalizers: []string{hcpVaultSecretsAppFinalizer},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:         common.OperatorNamespace,
+					Name:              "shadowSecret2",
+					DeletionTimestamp: &deletionTimestamp,
+					Finalizers:        []string{vaultDynamicSecretFinalizer},
+					Labels: map[string]string{
+						"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:      "shadowSecret3",
+					Labels: map[string]string{
+						"app.kubernetes.io/component": "hvs-dynamic-secret-cache",
+					},
+					DeletionTimestamp: &deletionTimestamp,
+					Finalizers:        []string{hcpVaultSecretsAppFinalizer},
+				},
+			},
+			isShadowSecretDeletionExpected: true,
+		},
+		"secret-not-dynamic": {
+			o: &secretsv1beta1.HCPVaultSecretsApp{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       HCPVaultSecretsApp.String(),
+					APIVersion: secretsv1beta1.GroupVersion.Version,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					UID:        "hvsApp4UID",
+					Namespace:  "hvsApp4Namespace",
+					Name:       "hvsApp4",
+					Finalizers: []string{hcpVaultSecretsAppFinalizer},
+				},
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace:         common.OperatorNamespace,
+					Name:              "nonShadowSecret",
+					DeletionTimestamp: &deletionTimestamp,
+					Finalizers:        []string{vaultDynamicSecretFinalizer},
+					Labels: map[string]string{
+						hvsaLabelPrefix + "/namespace": "hvsApp4Namespace",
+						hvsaLabelPrefix + "/name":      "hvsApp4",
+						helpers.LabelOwnerRefUID:       "hvsApp4UID",
+					},
+				},
+			},
+			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))
+			}
+
+			if tt.isShadowSecretDeletionExpected {
+				deletedSecret := &corev1.Secret{}
+				err := r.Get(ctx, makeShadowObjKey(tt.secret), deletedSecret)
+				assert.True(t, apierrors.IsNotFound(err))
+			} else {
+				secret := &corev1.Secret{}
+				err := r.Get(ctx, ctrlclient.ObjectKeyFromObject(tt.secret), secret)
+				assert.False(t, apierrors.IsNotFound(err))
+			}
+		})
+	}
+}

helpers/secrets.go

@@ -41,7 +41,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
@@ -66,7 +66,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
 }
@@ -84,7 +84,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
 }

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

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"`
+
+	// CleanupOrphanedShadowSecretInterval is VSO_HVSA_CLEANUP_ORPHANED_SHADOW_SECRETS_INTERVAL environment variable option
+	HVSACleanupOrphanedShadowSecretInterval time.Duration `split_words:"true"`
 }
 
 // Parse environment variable options, prefixed with "VSO_"