From 1a95fa410ba6bfbcc40c2d22d6c93ea4f60bee21 Mon Sep 17 00:00:00 2001 From: Jaired Jawed Date: Wed, 29 Jan 2025 15:34:34 -0800 Subject: [PATCH] include changes to satisfy comments --- controllers/hcpvaultsecretsapp_controller.go | 70 +++++++++++--------- internal/options/env.go | 4 +- internal/options/env_test.go | 52 +++++++-------- main.go | 25 ++++--- 4 files changed, 77 insertions(+), 74 deletions(-) diff --git a/controllers/hcpvaultsecretsapp_controller.go b/controllers/hcpvaultsecretsapp_controller.go index 328ef7e6..f8c0f584 100644 --- a/controllers/hcpvaultsecretsapp_controller.go +++ b/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 })) diff --git a/internal/options/env.go b/internal/options/env.go index df9a7edc..6d25ae39 100644 --- a/internal/options/env.go +++ b/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_" diff --git a/internal/options/env_test.go b/internal/options/env_test.go index 0585c4f2..c5e626e4 100644 --- a/internal/options/env_test.go +++ b/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, }, }, } diff --git a/main.go b/main.go index 2dc3180c..6da1c475 100644 --- a/main.go +++ b/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) }