Add tests to use rotation config.

Author: dargudear-google

controllers/secretproviderclasspodstatus_controller.go

@@ -295,58 +295,44 @@ func (r *SecretProviderClassPodStatusReconciler) Reconcile(ctx context.Context,
 			errs = append(errs, fmt.Errorf("failed to validate secret object in spc %s/%s, err: %w", spc.Namespace, spc.Name, err))
 			continue
 		}
-		exists, err := r.secretExists(ctx, secretName, req.Namespace)
-		if err != nil {
-			klog.ErrorS(err, "failed to check if secret exists", "secret", klog.ObjectRef{Namespace: req.Namespace, Name: secretName}, "spc", klog.KObj(spc), "pod", klog.KObj(pod), "spcps", klog.KObj(spcPodStatus))
-			// 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) {
-				klog.Warning(SyncSecretForbiddenWarning)
-			}
-			errs = append(errs, fmt.Errorf("failed to check if secret %s exists, err: %w", secretName, err))
-			continue
-		}
 
 		var funcs []func() (bool, error)
+		secretType := secretutil.GetSecretType(strings.TrimSpace(secretObj.Type))
 
-		if !exists {
-			secretType := secretutil.GetSecretType(strings.TrimSpace(secretObj.Type))
-
-			var datamap map[string][]byte
-			if datamap, err = secretutil.GetSecretData(secretObj.Data, secretType, files); err != nil {
-				r.generateEvent(pod, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err))
-				klog.ErrorS(err, "failed to get data in spc for secret", "spc", klog.KObj(spc), "pod", klog.KObj(pod), "secret", klog.ObjectRef{Namespace: req.Namespace, Name: secretName}, "spcps", klog.KObj(spcPodStatus))
-				errs = append(errs, fmt.Errorf("failed to get data in spc %s/%s for secret %s, err: %w", req.Namespace, spcName, secretName, err))
-				continue
-			}
+		var datamap map[string][]byte
+		if datamap, err = secretutil.GetSecretData(secretObj.Data, secretType, files); err != nil {
+			r.generateEvent(pod, corev1.EventTypeWarning, secretCreationFailedReason, fmt.Sprintf("failed to get data in spc %s/%s for secret %s, err: %+v", req.Namespace, spcName, secretName, err))
+			klog.ErrorS(err, "failed to get data in spc for secret", "spc", klog.KObj(spc), "pod", klog.KObj(pod), "secret", klog.ObjectRef{Namespace: req.Namespace, Name: secretName}, "spcps", klog.KObj(spcPodStatus))
+			errs = append(errs, fmt.Errorf("failed to get data in spc %s/%s for secret %s, err: %w", req.Namespace, spcName, secretName, err))
+			continue
+		}
 
-			labelsMap := make(map[string]string)
-			if secretObj.Labels != nil {
-				labelsMap = secretObj.Labels
-			}
-			annotationsMap := make(map[string]string)
-			if secretObj.Annotations != nil {
-				annotationsMap = secretObj.Annotations
-			}
-			// Set secrets-store.csi.k8s.io/managed=true label on the secret that's created and managed
-			// by the secrets-store-csi-driver. This label will be used to perform a filtered list watch
-			// only on secrets created and managed by the driver
-			labelsMap[SecretManagedLabel] = "true"
-
-			createFn := func() (bool, error) {
-				if err := r.createK8sSecret(ctx, secretName, req.Namespace, datamap, labelsMap, annotationsMap, secretType); err != nil {
-					klog.ErrorS(err, "failed to create Kubernetes secret", "spc", klog.KObj(spc), "pod", klog.KObj(pod), "secret", klog.ObjectRef{Namespace: req.Namespace, Name: secretName}, "spcps", klog.KObj(spcPodStatus))
-					// 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) {
-						klog.Warning(SyncSecretForbiddenWarning)
-					}
-					return false, nil
+		labelsMap := make(map[string]string)
+		if secretObj.Labels != nil {
+			labelsMap = secretObj.Labels
+		}
+		annotationsMap := make(map[string]string)
+		if secretObj.Annotations != nil {
+			annotationsMap = secretObj.Annotations
+		}
+		// Set secrets-store.csi.k8s.io/managed=true label on the secret that's created and managed
+		// by the secrets-store-csi-driver. This label will be used to perform a filtered list watch
+		// only on secrets created and managed by the driver
+		labelsMap[SecretManagedLabel] = "true"
+
+		createFn := func() (bool, error) {
+			if err := r.createOrUpdateK8sSecret(ctx, secretName, req.Namespace, datamap, labelsMap, annotationsMap, secretType); err != nil {
+				klog.ErrorS(err, "failed to create Kubernetes secret", "spc", klog.KObj(spc), "pod", klog.KObj(pod), "secret", klog.ObjectRef{Namespace: req.Namespace, Name: secretName}, "spcps", klog.KObj(spcPodStatus))
+				// 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) {
+					klog.Warning(SyncSecretForbiddenWarning)
 				}
-				return true, nil
+				return false, nil
 			}
-			funcs = append(funcs, createFn)
+			return true, nil
 		}
+		funcs = append(funcs, createFn)
 
 		for _, f := range funcs {
 			if err := wait.ExponentialBackoff(wait.Backoff{
@@ -410,9 +396,9 @@ func (r *SecretProviderClassPodStatusReconciler) processIfBelongsToNode(objMeta
 	return true
 }
 
-// createK8sSecret creates K8s secret with data from mounted files
+// createOrUpdateK8sSecret creates K8s secret with data from mounted files
 // If a secret with the same name already exists in the namespace of the pod, the error is nil.
-func (r *SecretProviderClassPodStatusReconciler) createK8sSecret(ctx context.Context, name, namespace string, datamap map[string][]byte, labelsmap map[string]string, annotationsmap map[string]string, secretType corev1.SecretType) error {
+func (r *SecretProviderClassPodStatusReconciler) createOrUpdateK8sSecret(ctx context.Context, name, namespace string, datamap map[string][]byte, labelsmap map[string]string, annotationsmap map[string]string, secretType corev1.SecretType) error {
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace:   namespace,
@@ -430,6 +416,13 @@ func (r *SecretProviderClassPodStatusReconciler) createK8sSecret(ctx context.Con
 		return nil
 	}
 	if apierrors.IsAlreadyExists(err) {
+		klog.InfoS("Kubernetes secret is already created", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
+		err := r.writer.Update(ctx, secret)
+		if err != nil {
+			klog.Errorf("Unable to update kubernetes secret", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
+			return err
+		}
+		klog.InfoS("successfully updated Kubernetes secret", "secret", klog.ObjectRef{Namespace: namespace, Name: name})
 		return nil
 	}
 	return err
@@ -477,23 +470,6 @@ func (r *SecretProviderClassPodStatusReconciler) patchSecretWithOwnerRef(ctx con
 	return nil
 }
 
-// secretExists checks if the secret with name and namespace already exists
-func (r *SecretProviderClassPodStatusReconciler) secretExists(ctx context.Context, name, namespace string) (bool, error) {
-	o := &corev1.Secret{}
-	secretKey := types.NamespacedName{
-		Namespace: namespace,
-		Name:      name,
-	}
-	err := r.Client.Get(ctx, secretKey, o)
-	if err == nil {
-		return true, nil
-	}
-	if apierrors.IsNotFound(err) {
-		return false, nil
-	}
-	return false, err
-}
-
 // generateEvent generates an event
 func (r *SecretProviderClassPodStatusReconciler) generateEvent(obj apiruntime.Object, eventType, reason, message string) {
 	if obj != nil {

controllers/secretproviderclasspodstatus_controller_test.go

@@ -121,31 +121,6 @@ func newReconciler(client client.Client, scheme *runtime.Scheme, nodeID string)
 	}
 }
 
-func TestSecretExists(t *testing.T) {
-	g := NewWithT(t)
-
-	scheme, err := setupScheme()
-	g.Expect(err).NotTo(HaveOccurred())
-
-	labels := map[string]string{"environment": "test"}
-	annotations := map[string]string{"kubed.appscode.com/sync": "app=test"}
-
-	initObjects := []client.Object{
-		newSecret("my-secret", "default", labels, annotations),
-	}
-
-	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjects...).Build()
-	reconciler := newReconciler(client, scheme, "node1")
-
-	exists, err := reconciler.secretExists(context.TODO(), "my-secret", "default")
-	g.Expect(exists).To(Equal(true))
-	g.Expect(err).NotTo(HaveOccurred())
-
-	exists, err = reconciler.secretExists(context.TODO(), "my-secret2", "default")
-	g.Expect(exists).To(Equal(false))
-	g.Expect(err).NotTo(HaveOccurred())
-}
-
 func TestPatchSecretWithOwnerRef(t *testing.T) {
 	g := NewWithT(t)
 
@@ -183,7 +158,7 @@ func TestPatchSecretWithOwnerRef(t *testing.T) {
 	g.Expect(secret.GetOwnerReferences()).To(HaveLen(1))
 }
 
-func TestCreateK8sSecret(t *testing.T) {
+func TestCreateOrUpdateK8sSecret(t *testing.T) {
 	g := NewWithT(t)
 
 	scheme, err := setupScheme()
@@ -198,11 +173,11 @@ func TestCreateK8sSecret(t *testing.T) {
 	client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjects...).Build()
 	reconciler := newReconciler(client, scheme, "node1")
 
-	// secret already exists
-	err = reconciler.createK8sSecret(context.TODO(), "my-secret", "default", nil, labels, annotations, corev1.SecretTypeOpaque)
+	// secret already exists, just update it.
+	err = reconciler.createOrUpdateK8sSecret(context.TODO(), "my-secret", "default", nil, labels, annotations, corev1.SecretTypeOpaque)
 	g.Expect(err).NotTo(HaveOccurred())
 
-	err = reconciler.createK8sSecret(context.TODO(), "my-secret2", "default", nil, labels, annotations, corev1.SecretTypeOpaque)
+	err = reconciler.createOrUpdateK8sSecret(context.TODO(), "my-secret2", "default", nil, labels, annotations, corev1.SecretTypeOpaque)
 	g.Expect(err).NotTo(HaveOccurred())
 	secret := &corev1.Secret{}
 	err = client.Get(context.TODO(), types.NamespacedName{Name: "my-secret2", Namespace: "default"}, secret)

deploy/csidriver.yaml

@@ -7,4 +7,3 @@ spec:
   attachRequired: false
   volumeLifecycleModes:
   - Ephemeral
-  requiresRepublish: true

manifest_staging/charts/secrets-store-csi-driver/templates/csidriver.yaml

@@ -11,6 +11,7 @@ spec:
   volumeLifecycleModes: 
   - Ephemeral
   {{- if and (semverCompare ">=1.20-0" .Capabilities.KubeVersion.Version) .Values.tokenRequests }}
+  requiresRepublish: true
   tokenRequests:
     {{- toYaml .Values.tokenRequests | nindent 2 }}
   {{- end }}

manifest_staging/deploy/csidriver.yaml

@@ -7,3 +7,4 @@ spec:
   attachRequired: false
   volumeLifecycleModes:
   - Ephemeral
+  requiresRepublish: true

pkg/secrets-store/nodeserver.go

@@ -76,14 +76,6 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	errorReason := internalerrors.FailedToMount
 	rotationEnabled := ns.rotationConfig.enabled
 
-	if ns.rotationConfig.enabled {
-		if ns.rotationConfig.nextRotationTime.After(startTime) {
-			klog.InfoS("Too soon !!!!, will rotate secret after", ns.rotationConfig.nextRotationTime)
-			return &csi.NodePublishVolumeResponse{}, nil
-		}
-		ns.rotationConfig.nextRotationTime = ns.rotationConfig.nextRotationTime.Add(ns.rotationConfig.interval)
-	}
-
 	defer func() {
 		if err != nil {
 			// if there is an error at any stage during node publish volume and if the path
@@ -127,6 +119,20 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	podNamespace = attrib[CSIPodNamespace]
 	podUID = attrib[CSIPodUID]
 
+	klog.InfoS("Checking object", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName})
+
+	if ns.rotationConfig.enabled {
+		lastModificationTime, err := ns.getLastUpdateTime(targetPath)
+		if err != nil {
+			klog.Infof("could not find last modification time for %s, error: %v\n", targetPath, err)
+		} else if startTime.Before(lastModificationTime.Add(ns.rotationConfig.rotationPollInterval)) {
+			// if next rotation is not yet due, then skip the mount operation
+			return &csi.NodePublishVolumeResponse{}, nil
+		}
+	}
+
+	klog.InfoS("Processing object", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName})
+
 	mounted, err = ns.ensureMountPoint(targetPath)
 	if err != nil {
 		// kubelet will not create the CSI NodePublishVolume target directory in 1.20+, in accordance with the CSI specification.
@@ -140,6 +146,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 			return nil, status.Errorf(codes.Internal, "failed to check if target path %s is mount point, err: %v", targetPath, err)
 		}
 	}
+	// If rotation is not enabled, don't remount the already mounted secrets.
 	if !rotationEnabled && mounted {
 		klog.InfoS("target path is already mounted", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName})
 		return &csi.NodePublishVolumeResponse{}, nil
@@ -196,7 +203,6 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	if parameters[CSIPodServiceAccountTokens] == "" {
 		// Inject pod service account token into volume attributes
 		klog.Error("csi.storage.k8s.io/serviceAccount.tokens is not populated, set RequiresRepublish")
-
 	}
 
 	// ensure it's read-only

pkg/secrets-store/nodeserver_test.go

@@ -21,6 +21,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	secretsstorev1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/secrets-store/mocks"
@@ -267,6 +268,7 @@ func TestNodePublishVolume(t *testing.T) {
 		name              string
 		nodePublishVolReq *csi.NodePublishVolumeRequest
 		initObjects       []client.Object
+		rotationConfig    *RotationConfig
 	}{
 		{
 			name: "volume mount",
@@ -294,9 +296,13 @@ func TestNodePublishVolume(t *testing.T) {
 					},
 				},
 			},
+			rotationConfig: &RotationConfig{
+				enabled:              false,
+				rotationPollInterval: time.Minute,
+			},
 		},
 		{
-			name: "volume mount with refresh token",
+			name: "volume mount with refresh token ",
 			nodePublishVolReq: &csi.NodePublishVolumeRequest{
 				VolumeCapability: &csi.VolumeCapability{},
 				VolumeId:         "testvolid1",
@@ -324,6 +330,41 @@ func TestNodePublishVolume(t *testing.T) {
 					},
 				},
 			},
+			rotationConfig: &RotationConfig{
+				enabled:              true,
+				rotationPollInterval: -1 * time.Minute, // Using negative interval to pass the rotation interval check in unit tests
+			},
+		},
+		{
+			name: "volume mount with rotation but skipped",
+			nodePublishVolReq: &csi.NodePublishVolumeRequest{
+				VolumeCapability: &csi.VolumeCapability{},
+				VolumeId:         "testvolid1",
+				TargetPath:       targetPath(t),
+				VolumeContext: map[string]string{
+					"secretProviderClass": "provider1",
+					CSIPodName:            "pod1",
+					CSIPodNamespace:       "default",
+					CSIPodUID:             "poduid1",
+				},
+				Readonly: true,
+			},
+			initObjects: []client.Object{
+				&secretsstorev1.SecretProviderClass{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "provider1",
+						Namespace: "default",
+					},
+					Spec: secretsstorev1.SecretProviderClassSpec{
+						Provider:   "provider1",
+						Parameters: map[string]string{"parameter1": "value1"},
+					},
+				},
+			},
+			rotationConfig: &RotationConfig{
+				enabled:              true,
+				rotationPollInterval: time.Minute,
+			},
 		},
 	}
 
@@ -338,7 +379,7 @@ func TestNodePublishVolume(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			r := mocks.NewFakeReporter()
 
-			ns, err := testNodeServer(t, fake.NewClientBuilder().WithScheme(s).WithObjects(test.initObjects...).Build(), r, &RotationConfig{})
+			ns, err := testNodeServer(t, fake.NewClientBuilder().WithScheme(s).WithObjects(test.initObjects...).Build(), r, test.rotationConfig)
 			if err != nil {
 				t.Fatalf("expected error to be nil, got: %+v", err)
 			}
@@ -365,8 +406,13 @@ func TestNodePublishVolume(t *testing.T) {
 				if err != nil {
 					t.Fatalf("expected err to be nil, got: %v", err)
 				}
-				if len(mnts) == 0 {
-					t.Errorf("expected mounts...: %v", mnts)
+				expectedMounts := 1
+				if ns.rotationConfig.enabled && ns.rotationConfig.rotationPollInterval > 0 {
+					// If due to rotation interval, NodePublishVolume has skipped, there should not be any mount operation
+					expectedMounts = 0
+				}
+				if len(mnts) != expectedMounts {
+					t.Errorf("[Number of mounts] want : %d, got mount: %d", expectedMounts, len(mnts))
 				}
 			}
 		})