fix review comments

Author: Suraiya-Hameed

controllers/secretproviderclasspodstatus_controller.go

@@ -272,17 +272,8 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(ctx context.Context,
 		return ctrl.Result{}, nil
 	}
 
+	// if SecretObjects defined in the SPC, record the time to report sync_k8s_secret_duration_sec metric
 	begin := time.Now()
-	providerName := string(spc.Spec.Provider)
-	namespace := spcPodStatus.Namespace
-	secretProviderClass := spc.Name
-	defer func() {
-		// if there is SecretObjects defined in the SPC, then report the metric if sync is successful
-		if e == nil && !res.Requeue {
-			r.reporter.ReportSyncSecretCtMetric(ctx, providerName, namespace, secretProviderClass)
-			r.reporter.ReportSyncSecretDuration(ctx, time.Since(begin).Seconds())
-		}
-	}()
 
 	// determine which pod volume this is associated with
 	podVol := k8sutil.SPCVolume(pod, r.driverName, spc.Name)
@@ -383,6 +374,9 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(ctx context.Context,
 		return ctrl.Result{Requeue: true}, nil
 	}
 
+	r.reporter.ReportSyncSecretCtMetric(ctx, string(spc.Spec.Provider), spcPodStatus.Namespace, spc.Name)
+	r.reporter.ReportSyncSecretDuration(ctx, time.Since(begin).Seconds())
+
 	klog.InfoS("reconcile complete", "spc", klog.KObj(spc), "pod", klog.KObj(pod), "spcps", klog.KObj(spcPodStatus))
 	// requeue the spc pod status again after 5mins to check if secret and ownerRef exists
 	// and haven't been modified. If secret doesn't exist, then this requeue will ensure it's

pkg/rotation/reconciler.go

@@ -269,14 +269,14 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 	err = r.cache.Get(
 		ctx,
 		client.ObjectKey{
-			Namespace: spcps.Namespace,
-			Name:      spcps.Status.PodName,
+			Namespace: podNamespace,
+			Name:      podName,
 		},
 		pod,
 	)
 	if err != nil {
 		errorReason = internalerrors.PodNotFound
-		return fmt.Errorf("failed to get pod %s/%s, err: %w", spcps.Namespace, spcps.Status.PodName, err)
+		return fmt.Errorf("failed to get pod %s/%s, err: %w", podNamespace, podName, err)
 	}
 	// skip rotation if the pod is being terminated
 	// or the pod is in succeeded state (for jobs that complete aren't gc yet)
@@ -292,14 +292,14 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 	err = r.cache.Get(
 		ctx,
 		client.ObjectKey{
-			Namespace: spcps.Namespace,
-			Name:      spcps.Status.SecretProviderClassName,
+			Namespace: podNamespace,
+			Name:      secretProviderClass,
 		},
 		spc,
 	)
 	if err != nil {
 		errorReason = internalerrors.SecretProviderClassNotFound
-		return fmt.Errorf("failed to get secret provider class %s/%s, err: %w", spcps.Namespace, spcps.Status.SecretProviderClassName, err)
+		return fmt.Errorf("failed to get secret provider class %s/%s, err: %w", podNamespace, secretProviderClass, err)
 	}
 
 	// determine which pod volume this is associated with
@@ -362,16 +362,16 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 	// This comprises the secret parameter in the MountRequest to the provider
 	if nodePublishSecretRef != nil {
 		// read secret from the informer cache
-		secret, err := r.secretStore.GetNodePublishSecretRefSecret(nodePublishSecretRef.Name, spcps.Namespace)
+		secret, err := r.secretStore.GetNodePublishSecretRefSecret(nodePublishSecretRef.Name, podNamespace)
 		if err != nil {
 			if apierrors.IsNotFound(err) {
 				klog.ErrorS(err,
-					fmt.Sprintf("nodePublishSecretRef not found. If the secret with name exists in namespace, label the secret by running 'kubectl label secret %s %s=true -n %s", nodePublishSecretRef.Name, controllers.SecretUsedLabel, spcps.Namespace),
-					"name", nodePublishSecretRef.Name, "namespace", spcps.Namespace)
+					fmt.Sprintf("nodePublishSecretRef not found. If the secret with name exists in namespace, label the secret by running 'kubectl label secret %s %s=true -n %s", nodePublishSecretRef.Name, controllers.SecretUsedLabel, podNamespace),
+					"name", nodePublishSecretRef.Name, "namespace", podNamespace)
 			}
 			errorReason = internalerrors.NodePublishSecretRefNotFound
-			r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("failed to get node publish secret %s/%s, err: %+v", spcps.Namespace, nodePublishSecretRef.Name, err))
-			return fmt.Errorf("failed to get node publish secret %s/%s, err: %w", spcps.Namespace, nodePublishSecretRef.Name, err)
+			r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("failed to get node publish secret %s/%s, err: %+v", podNamespace, nodePublishSecretRef.Name, err))
+			return fmt.Errorf("failed to get node publish secret %s/%s, err: %w", podNamespace, nodePublishSecretRef.Name, err)
 		}
 
 		for k, v := range secret.Data {
@@ -404,7 +404,7 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 	newObjectVersions, errorReason, err := secretsstore.MountContent(ctx, providerClient, string(paramsJSON), string(secretsJSON), spcps.Status.TargetPath, string(permissionJSON), oldObjectVersions)
 	if err != nil {
 		r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("provider mount err: %+v", err))
-		return fmt.Errorf("failed to rotate objects for pod %s/%s, err: %w", spcps.Namespace, spcps.Status.PodName, err)
+		return fmt.Errorf("failed to rotate objects for pod %s/%s, err: %w", podNamespace, podName, err)
 	}
 
 	// compare the old object versions and new object versions to check if any of the objects
@@ -491,7 +491,7 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 
 		patchFn := func() (bool, error) {
 			// patch secret data with the new contents
-			if err := r.patchSecret(ctx, secretObj.SecretName, spcps.Namespace, datamap); err != nil {
+			if err := r.patchSecret(ctx, secretObj.SecretName, podNamespace, datamap); err != nil {
 				// syncSecret.enabled is set to false by default in the helm chart for installing the driver in v0.0.23+
 				// that would result in a forbidden error, so generate a warning that can be helpful for debugging
 				if apierrors.IsForbidden(err) {

pkg/secrets-store/nodeserver_test.go

@@ -24,9 +24,8 @@ import (
 	"testing"
 	"time"
 
-	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
-
 	secretsstorev1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1"
+	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/k8s"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/secrets-store/mocks"
 	providerfake "sigs.k8s.io/secrets-store-csi-driver/provider/fake"