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. Change the GID and FSGroup to generic int to allow for 64bit int if and when applicable
   as provisioned at https://github.com/kubernetes/kubernetes/blob/b910026535af2d8a64d45efefeb8d9efb75a4817/pkg/volume/csi/csi_client.go#L64
5. Fixed enable_secret_rotation in e2e-provider.bats to correctly return the name of the pod.

Author: mytreya-rh

pkg/constants/constants.go

@@ -350,7 +350,7 @@ func (ns *nodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag
 	return &csi.NodeUnstageVolumeResponse{}, nil
 }
 
-func (ns *nodeServer) mountSecretsStoreObjectContent(ctx context.Context, providerName, attributes, secrets, targetPath, permission, podName string, gid int64) (map[string]string, string, error) {
+func (ns *nodeServer) mountSecretsStoreObjectContent(ctx context.Context, providerName, attributes, secrets, targetPath, permission, podName string, gid int) (map[string]string, string, error) {
 	if len(attributes) == 0 {
 		return nil, "", errors.New("missing attributes")
 	}

pkg/secrets-store/nodeserver.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

@@ -225,7 +225,7 @@ func (p *PluginClientBuilder) HealthCheck(ctx context.Context, interval time.Dur
 
 // MountContent calls the client's Mount() RPC with helpers to format the
 // request and interpret the response.
-func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient, attributes, secrets, targetPath, permission string, oldObjectVersions map[string]string, gid int64) (map[string]string, string, error) {
+func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient, attributes, secrets, targetPath, permission string, oldObjectVersions map[string]string, gid int) (map[string]string, string, error) {
 	var objVersions []*v1alpha1.ObjectVersion
 	for obj, version := range oldObjectVersions {
 		objVersions = append(objVersions, &v1alpha1.ObjectVersion{Id: obj, Version: version})

pkg/secrets-store/provider_client.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

@@ -71,7 +71,7 @@ type AtomicWriter struct {
 type FileProjection struct {
 	Data    []byte
 	Mode    int32
-	FsGroup *int64
+	FsGroup *int
 }
 
 // NewAtomicWriter creates a new AtomicWriter configured to write to the given
@@ -415,8 +415,8 @@ func (w *AtomicWriter) writePayloadToDir(payload map[string]FileProjection, dir
 		if fileProjection.FsGroup == nil || runtimeutil.IsRuntimeWindows() {
 			continue
 		}
-		if err := os.Chown(fullPath, -1, int(*fileProjection.FsGroup)); err != nil {
-			klog.ErrorS(err, "unable to change file with owner", "logContext", w.logContext, "fullPath", fullPath, "owner", int(*fileProjection.FsGroup))
+		if err := os.Chown(fullPath, -1, *fileProjection.FsGroup); err != nil {
+			klog.ErrorS(err, "unable to change file with owner", "logContext", w.logContext, "fullPath", fullPath, "owner", *fileProjection.FsGroup)
 			return err
 		}
 	}

pkg/util/fileutil/atomic_writer.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 int = -1
 )
 
 var (
@@ -131,14 +134,11 @@ func GetVolumeNameFromTargetPath(targetPath string) string {
 	return match[2]
 }
 
-// ParseFSGroup parses the FSGroup string and returns the GID as int64.
-// If fsGroupStr is empty, returns constants.NoGID.
-// Returns an error if the fsGroupStr cannot be parsed as a valid non-negative int64.
-func ParseFSGroup(fsGroupStr string) (int64, error) {
+// ParseFSGroup parses the FSGroup string and returns the GID.
+// If fsGroupStr is empty, returns NoGID.
+func ParseFSGroup(fsGroupStr string) (int, error) {
 	if len(fsGroupStr) == 0 {
-		return constants.NoGID, nil
+		return NoGID, nil
 	}
-	// Non-sentinel negative GID is invalid and thus we use ParseUint here.
-	gid, err := strconv.ParseUint(fsGroupStr, 10, 63)
-	return int64(gid), err
+	return strconv.Atoi(fsGroupStr)
 }

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) {
@@ -339,13 +338,13 @@ func TestParseFSGroup(t *testing.T) {
 	cases := []struct {
 		name        string
 		fsGroupStr  string
-		want        int64
+		want        int
 		expectedErr bool
 	}{
 		{
 			name:        "empty string returns NoGID",
 			fsGroupStr:  "",
-			want:        constants.NoGID,
+			want:        NoGID,
 			expectedErr: false,
 		},
 		{
@@ -382,9 +381,9 @@ func TestParseFSGroup(t *testing.T) {
 			expectedErr: true,
 		},
 		{
-			name:        "negative gid",
-			fsGroupStr:  "-23",
-			expectedErr: true,
+			name:       "negative gid",
+			fsGroupStr: "-23",
+			want:       -23,
 		},
 	}
 

pkg/util/fileutil/filesystem_test.go

@@ -42,7 +42,7 @@ func Validate(payloads []*v1alpha1.File) error {
 // WritePayloads writes the files to target directory. This helper builds the
 // atomic writer and converts the v1alpha1.File proto to the FileProjection type
 // used by the atomic writer.
-func WritePayloads(path string, payloads []*v1alpha1.File, gid int64) error {
+func WritePayloads(path string, payloads []*v1alpha1.File, gid int) error {
 	if err := Validate(payloads); err != nil {
 		return err
 	}
@@ -58,13 +58,18 @@ func WritePayloads(path string, payloads []*v1alpha1.File, gid int64) error {
 		return err
 	}
 
+	var fsGroup *int
+	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)
 	}