Address the latest review comments by:

1. delete constants.go and move NoGID to fileutil
2. Re-include "volume mount with rotation but skipped" in nodeserver_test.go
3. Added comment about why 64 bit word was chosen for FsGroup in filesystem.go
4. Populate FSGroup attribute to non-nil only if the GID is a valid value in writer.go
5. Fixed enable_secret_rotation in e2e-provider.bats to correctly return the name of the pod.

Author: mytreya-rh

pkg/constants/constants.go

@@ -91,6 +91,7 @@ func getRequest(t *testing.T, customize func(*csi.NodePublishVolumeRequest)) *cs
 	customize(request)
 	return request
 }
+
 func TestNodePublishVolume_Errors(t *testing.T) {
 	tests := []struct {
 		name              string
@@ -268,6 +269,15 @@ func TestNodePublishVolume(t *testing.T) {
 				rotationCacheDuration: -1 * time.Minute, // Using negative interval to pass the rotation interval check in unit tests
 			},
 		},
+		{
+			name:              "volume mount with rotation but skipped",
+			nodePublishVolReq: getRequest(t, func(*csi.NodePublishVolumeRequest) {}),
+			initObjects:       getInitObjects(func(*secretsstorev1.SecretProviderClass) {}),
+			rotationConfig: &rotationConfig{
+				enabled:               true,
+				rotationCacheDuration: time.Minute,
+			},
+		},
 		{
 			name: "volume mount with valid FSGroup",
 			nodePublishVolReq: getRequest(t, func(r *csi.NodePublishVolumeRequest) {

pkg/secrets-store/nodeserver_test.go

@@ -27,7 +27,6 @@ import (
 	"testing"
 	"time"
 
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/fileutil"
 	"sigs.k8s.io/secrets-store-csi-driver/provider/fake"
 	"sigs.k8s.io/secrets-store-csi-driver/provider/v1alpha1"
@@ -196,7 +195,7 @@ func TestMountContent(t *testing.T) {
 				t.Fatalf("expected err to be nil, got: %+v", err)
 			}
 
-			objectVersions, _, err := MountContent(context.TODO(), client, "{}", "{}", targetPath, test.permission, nil, constants.NoGID)
+			objectVersions, _, err := MountContent(context.TODO(), client, "{}", "{}", targetPath, test.permission, nil, fileutil.NoGID)
 			if err != nil {
 				t.Errorf("expected err to be nil, got: %+v", err)
 			}
@@ -254,7 +253,7 @@ func TestMountContent_TooLarge(t *testing.T) {
 	}
 
 	// rpc error: code = ResourceExhausted desc = grpc: received message larger than max (28 vs. 5)
-	_, errorCode, err := MountContent(context.TODO(), client, "{}", "{}", targetPath, "777", nil, constants.NoGID)
+	_, errorCode, err := MountContent(context.TODO(), client, "{}", "{}", targetPath, "777", nil, fileutil.NoGID)
 	if err == nil {
 		t.Errorf("expected err to be not nil")
 	}
@@ -348,7 +347,7 @@ func TestMountContentError(t *testing.T) {
 				t.Fatalf("expected err to be nil, got: %+v", err)
 			}
 
-			objectVersions, errorCode, err := MountContent(context.TODO(), client, test.attributes, test.secrets, test.targetPath, test.permission, nil, constants.NoGID)
+			objectVersions, errorCode, err := MountContent(context.TODO(), client, test.attributes, test.secrets, test.targetPath, test.permission, nil, fileutil.NoGID)
 			if err == nil {
 				t.Errorf("expected err to be not nil")
 			}

pkg/secrets-store/provider_client_test.go

@@ -23,8 +23,11 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+)
 
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
+const (
+	// NoGID is the default gid -1 to indicate no change in FSGroup
+	NoGID = int64(-1)
 )
 
 var (
@@ -132,13 +135,15 @@ func GetVolumeNameFromTargetPath(targetPath string) string {
 }
 
 // ParseFSGroup parses the FSGroup string and returns the GID as int64.
-// If fsGroupStr is empty, returns constants.NoGID.
+// If fsGroupStr is empty, returns NoGID.
 // Returns an error if the fsGroupStr cannot be parsed as a valid non-negative int64.
 func ParseFSGroup(fsGroupStr string) (int64, error) {
 	if len(fsGroupStr) == 0 {
-		return constants.NoGID, nil
+		return NoGID, nil
 	}
 	// Non-sentinel negative GID is invalid and thus we use ParseUint here.
+	// Though the Linux GID is 32-bit, we use 64 bit int to align with the nodePublishVolume convention for FsGroup
+	// please see: https://github.com/kubernetes/kubernetes/blob/b910026535af2d8a64d45efefeb8d9efb75a4817/pkg/volume/csi/csi_client.go#L64
 	gid, err := strconv.ParseUint(fsGroupStr, 10, 63)
 	return int64(gid), err
 }

pkg/util/fileutil/filesystem.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 )
 
 func TestGetMountedFiles(t *testing.T) {
@@ -345,7 +344,7 @@ func TestParseFSGroup(t *testing.T) {
 		{
 			name:        "empty string returns NoGID",
 			fsGroupStr:  "",
-			want:        constants.NoGID,
+			want:        NoGID,
 			expectedErr: false,
 		},
 		{

pkg/util/fileutil/filesystem_test.go

@@ -58,13 +58,18 @@ func WritePayloads(path string, payloads []*v1alpha1.File, gid int64) error {
 		return err
 	}
 
+	var fsGroup *int64
+	if gid != NoGID {
+		fsGroup = &gid
+	}
+
 	// convert v1alpha1.File to FileProjection
 	files := make(map[string]FileProjection, len(payloads))
 	for _, payload := range payloads {
 		files[payload.GetPath()] = FileProjection{
 			Data:    payload.GetContents(),
 			Mode:    payload.GetMode(),
-			FsGroup: &gid,
+			FsGroup: fsGroup,
 		}
 	}
 

pkg/util/fileutil/writer.go

@@ -27,7 +27,6 @@ import (
 	"strings"
 	"testing"
 
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/runtimeutil"
 	"sigs.k8s.io/secrets-store-csi-driver/provider/v1alpha1"
 )
@@ -373,7 +372,7 @@ func TestWritePayloads(t *testing.T) {
 			dir := t.TempDir()
 
 			// check that the first write succeeds and the contents match
-			if err := WritePayloads(dir, tc.first, constants.NoGID); err != nil {
+			if err := WritePayloads(dir, tc.first, NoGID); err != nil {
 				t.Errorf("WritePayload(first) got error: %v", err)
 			}
 
@@ -383,7 +382,7 @@ func TestWritePayloads(t *testing.T) {
 
 			// check that the second write succeeds and the contents match,
 			// ensuring that the files have the updated values
-			if err := WritePayloads(dir, tc.second, constants.NoGID); err != nil {
+			if err := WritePayloads(dir, tc.second, NoGID); err != nil {
 				t.Errorf("WritePayload(second) got error: %v", err)
 			}
 
@@ -422,7 +421,7 @@ func TestWritePayloads_BackwardCompatible(t *testing.T) {
 
 	want := []byte("new")
 
-	if err := WritePayloads(dir, payload, constants.NoGID); err != nil {
+	if err := WritePayloads(dir, payload, NoGID); err != nil {
 		t.Fatalf("could not write new file: %s", err)
 	}
 

pkg/util/fileutil/writer_test.go

@@ -13,7 +13,7 @@ fi
 
 # export secret vars
 export SECRET_NAME=${SECRET_NAME:-foo}
-# defualt version value returned by mock provider
+# default version value returned by mock provider
 export SECRET_VERSION=${SECRET_VERSION:-"v1"}
 # default secret value returned by the mock provider
 export SECRET_VALUE=${SECRET_VALUE:-"secret"}
@@ -22,12 +22,12 @@ export SECRET_MODE=${SECRET_MODE:-'"0644"'}
 
 # export key vars
 export KEY_NAME=${KEY_NAME:-fookey}
-# defualt version value returned by mock provider
+# default version value returned by mock provider
 export KEY_VERSION=${KEY_VERSION:-"v1"}
 # default key value returned by mock provider.
 # base64 encoded content comparision is easier in case of very long multiline string.
 export KEY_VALUE_CONTAINS=${KEY_VALUE:-"LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KVGhpcyBpcyBtb2NrIGtleQotLS0tLUVORCBQVUJMSUMgS0VZLS0tLS0K"}
-# defualt version value returned by mock provider
+# default version value returned by mock provider
 export KEY_MODE=${KEY_MODE:-'"0644"'}
 
 # export node selector var
@@ -98,19 +98,20 @@ function delete_pod() {
   # On Windows, the failed unmount calls from: https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/545
   # do not prevent the pod from being deleted. Search through the driver logs
   # for the error.
-  run bash -c "kubectl -n $NAMESPACE logs -l app=$POD_NAME --tail -1 -c secrets-store -n kube-system | grep '^E.*failed to clean and unmount target path.*$'"
+  run bash -c "kubectl -n $NAMESPACE logs -l app=$POD_NAME --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'"
   assert_failure
 }
 
 function enable_secret_rotation() {
   # enable rotation response in mock server
   local curl_pod_name=curl-$(openssl rand -hex 5)
-  kubectl run ${curl_pod_name} -n rotation --image=curlimages/curl:7.75.0 --labels="test=rotation" -- tail -f /dev/null
-  kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name}
+  kubectl run ${curl_pod_name} -n rotation --image=curlimages/curl:7.75.0 --labels="test=rotation" -- tail -f /dev/null > /dev/null
+  kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null
   local pod_ip=$(kubectl get pod -n kube-system -l app=csi-secrets-store-e2e-provider -o jsonpath="{.items[0].status.podIP}")
   run kubectl exec ${curl_pod_name} -n rotation -- curl http://${pod_ip}:8080/rotation?rotated=true
   # wait for rotated secret to be mounted
   sleep 120
+  echo "${curl_pod_name}"
 }
 
 function disable_secret_rotation() {