include changes to satisfy comments

Author: jaireddjawed

controllers/hcpvaultsecretsapp_controller.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -83,16 +84,15 @@ var (
 // HCPVaultSecretsAppReconciler reconciles a HCPVaultSecretsApp object
 type HCPVaultSecretsAppReconciler struct {
 	client.Client
-	Scheme                              *runtime.Scheme
-	Recorder                            record.EventRecorder
-	SecretDataBuilder                   *helpers.SecretDataBuilder
-	HMACValidator                       helpers.HMACValidator
-	MinRefreshAfter                     time.Duration
-	referenceCache                      ResourceReferenceCache
-	GlobalTransformationOptions         *helpers.GlobalTransformationOptions
-	BackOffRegistry                     *BackOffRegistry
-	CleanupOrphanedShadowSecretInterval time.Duration
-	once                                sync.Once
+	Scheme                      *runtime.Scheme
+	Recorder                    record.EventRecorder
+	SecretDataBuilder           *helpers.SecretDataBuilder
+	HMACValidator               helpers.HMACValidator
+	MinRefreshAfter             time.Duration
+	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
@@ -293,7 +293,7 @@ func (r *HCPVaultSecretsAppReconciler) Reconcile(ctx context.Context, req ctrl.R
 	}, nil
 }
 
-func (r *HCPVaultSecretsAppReconciler) startOrphanedShadowSecretCleanup(ctx context.Context) error {
+func (r *HCPVaultSecretsAppReconciler) startOrphanedShadowSecretCleanup(ctx context.Context, cleanupOrphanedShadowSecretInterval time.Duration) error {
 	var err error
 
 	r.once.Do(func() {
@@ -303,62 +303,66 @@ func (r *HCPVaultSecretsAppReconciler) startOrphanedShadowSecretCleanup(ctx cont
 				if ctx.Err() != nil {
 					err = ctx.Err()
 				}
+				break
 			// runs the cleanup process once every hour or as specified by the user
-			case <-time.After(r.CleanupOrphanedShadowSecretInterval):
-				err = r.cleanupOrphanedShadowSecrets(ctx)
+			case <-time.After(cleanupOrphanedShadowSecretInterval):
+				r.cleanupOrphanedShadowSecrets(ctx)
 			}
 		}
 	})
 
-	return fmt.Errorf("shadow secret cleanup error err=%s", err)
+	return err
 }
 
-func (r *HCPVaultSecretsAppReconciler) cleanupOrphanedShadowSecrets(ctx context.Context) error {
+func (r *HCPVaultSecretsAppReconciler) cleanupOrphanedShadowSecrets(ctx context.Context) {
 	logger := log.FromContext(ctx).WithName("cleanupOrphanedShadowSecrets")
+	var errs error
 
-	// filtering only for dynamic secrets
-	dynamicSecretLabelSelector := client.MatchingLabels{"app.kubernetes.io/component": "hvs-dynamic-secret-cache"}
+	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), dynamicSecretLabelSelector); err != nil {
-		logger.Error(err, "Failed to list shadow secrets")
-		return err
+	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 {
-		o := &secretsv1beta1.HCPVaultSecretsApp{}
+		namespace := secret.Labels[namespaceLabelKey]
+		name := secret.Labels[nameLabelKey]
+		objKey := types.NamespacedName{Namespace: namespace, Name: name}
 
-		namespace := secret.Labels[hvsaLabelPrefix+"/namespace"]
-		name := secret.Labels[hvsaLabelPrefix+"/name"]
-		namespacedName := types.NamespacedName{Namespace: namespace, Name: name}
+		o := &secretsv1beta1.HCPVaultSecretsApp{}
 
-		// get the HCPVaultSecretsApp instance that that the shadow secret belongs to (if applicable)
+		// 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, namespacedName, o)
+		err := r.Get(ctx, objKey, o)
 		if err != nil && !apierrors.IsNotFound(err) {
-			logger.Error(err, "Error getting resource from k8s", "secret", secret.Name)
+			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 {
-				logger.Error(err, "Failed to handle deletion of HCPVaultSecretsApp", "app", o.Name)
+				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, namespacedName); err != nil {
-				logger.Error(err, "Failed to delete shadow secret", "secret", secret.Name)
+			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)
 		}
 	}
 
-	return nil
+	logger.Error(errs, "Failed during cleanup of orphaned shadow secrets")
 }
 
 func (r *HCPVaultSecretsAppReconciler) updateStatus(ctx context.Context, o *secretsv1beta1.HCPVaultSecretsApp) error {
@@ -373,14 +377,14 @@ 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)
+		err := r.startOrphanedShadowSecretCleanup(ctx, cleanupOrphanedShadowSecretInterval)
 		return err
 	}))
 

internal/options/env.go

@@ -50,8 +50,8 @@ type VSOEnvOptions struct {
 	// ClientCacheNumLocks is VSO_CLIENT_CACHE_NUM_LOCKS environment variable option
 	ClientCacheNumLocks *int `split_words:"true"`
 
-	// CleanupOrphanedShadowSecretInterval is VSO_CLEANUP_ORPHANED_SHADOW_SECRETS_INTERVAL environment variable option
-	CleanupOrphanedShadowSecretInterval time.Duration `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_"

internal/options/env_test.go

@@ -23,34 +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_CLEANUP_ORPHANED_SHADOW_SECRET_INTERVAL": "1h",
+				"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_HVSA_CLEANUP_ORPHANED_SHADOW_SECRET_INTERVAL": "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),
-				CleanupOrphanedShadowSecretInterval: time.Hour * 1,
+				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),
+				HVSACleanupOrphanedShadowSecretInterval: time.Hour * 1,
 			},
 		},
 	}

main.go

@@ -218,7 +218,7 @@ func main() {
 	flag.DurationVar(&cleanupOrphanedShadowSecretInterval, "cleanup-orphaned-shadow-secrets-interval", 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_ORPHANED_SHADOW_SECRET_INTERVAL.")
+			"Also set from environment variable VSO_HVSA_CLEANUP_ORPHANED_SHADOW_SECRET_INTERVAL.")
 
 	opts := zap.Options{
 		Development: os.Getenv("VSO_LOGGER_DEVELOPMENT_MODE") != "",
@@ -267,8 +267,8 @@ func main() {
 	if vsoEnvOptions.BackoffMultiplier != 0 {
 		backoffMultiplier = vsoEnvOptions.BackoffMultiplier
 	}
-	if vsoEnvOptions.CleanupOrphanedShadowSecretInterval != 0 {
-		cleanupOrphanedShadowSecretInterval = vsoEnvOptions.CleanupOrphanedShadowSecretInterval
+	if vsoEnvOptions.HVSACleanupOrphanedShadowSecretInterval != 0 {
+		cleanupOrphanedShadowSecretInterval = vsoEnvOptions.HVSACleanupOrphanedShadowSecretInterval
 	}
 	if len(vsoEnvOptions.GlobalVaultAuthOptions) > 0 {
 		globalVaultAuthOptsSet = vsoEnvOptions.GlobalVaultAuthOptions
@@ -555,16 +555,15 @@ func main() {
 		os.Exit(1)
 	}
 	if err = (&controllers.HCPVaultSecretsAppReconciler{
-		Client:                              mgr.GetClient(),
-		Scheme:                              mgr.GetScheme(),
-		Recorder:                            mgr.GetEventRecorderFor("HCPVaultSecretsApp"),
-		SecretDataBuilder:                   secretDataBuilder,
-		HMACValidator:                       hmacValidator,
-		MinRefreshAfter:                     minRefreshAfterHVSA,
-		BackOffRegistry:                     controllers.NewBackOffRegistry(backoffOpts...),
-		GlobalTransformationOptions:         globalTransOptions,
-		CleanupOrphanedShadowSecretInterval: cleanupOrphanedShadowSecretInterval,
-	}).SetupWithManager(mgr, controllerOptions); err != nil {
+		Client:                      mgr.GetClient(),
+		Scheme:                      mgr.GetScheme(),
+		Recorder:                    mgr.GetEventRecorderFor("HCPVaultSecretsApp"),
+		SecretDataBuilder:           secretDataBuilder,
+		HMACValidator:               hmacValidator,
+		MinRefreshAfter:             minRefreshAfterHVSA,
+		BackOffRegistry:             controllers.NewBackOffRegistry(backoffOpts...),
+		GlobalTransformationOptions: globalTransOptions,
+	}).SetupWithManager(mgr, controllerOptions, cleanupOrphanedShadowSecretInterval); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "HCPVaultSecretsApp")
 		os.Exit(1)
 	}