Addressed review comments:

1. pkg/constants/constants.go
   - Added new line after header comment
   - Moved inline comment for var NoGID to an above the code comment.
2. pkg/rotation/reconciler.go
   pkg/secrets-store/nodeserver.go
   - Now use common function: fileutil.ParseFSGroup instead of ParseInt directly
   - Un-necessary logs removed.
3. pkg/secrets-store/utils.go
   - Removed a not so useful log
4. pkg/util/fileutil/filesystem.go
   - Defines common function ParseFSGroup used by reconcile and nodeserver
5. pkg/util/fileutil/filesystem_test.go
   - Tests ParseFSGroup

Author: mytreya-rh

pkg/constants/constants.go

@@ -13,8 +13,10 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
+
 package constants
 
 const (
-	NoGID = int64(-1) // Use the default gid -1 to indicate no change in FSGroup
+	// NoGID is the default gid -1 to indicate no change in FSGroup
+	NoGID = int64(-1)
 )

pkg/rotation/reconciler.go

@@ -21,14 +21,12 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"strconv"
 	"strings"
 	"time"
 
 	secretsstorev1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1"
 	"sigs.k8s.io/secrets-store-csi-driver/controllers"
 	secretsStoreClient "sigs.k8s.io/secrets-store-csi-driver/pkg/client/clientset/versioned"
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
 	"sigs.k8s.io/secrets-store-csi-driver/pkg/k8s"
 	secretsstore "sigs.k8s.io/secrets-store-csi-driver/pkg/secrets-store"
@@ -400,15 +398,12 @@ func (r *Reconciler) reconcile(ctx context.Context, spcps *secretsstorev1.Secret
 		r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("failed to lookup provider client: %q", providerName))
 		return fmt.Errorf("failed to lookup provider client: %q", providerName)
 	}
-	gid := constants.NoGID
-	if len(spcps.Status.FSGroup) > 0 {
-		gid, err = strconv.ParseInt(spcps.Status.FSGroup, 10, 64)
-		if err != nil {
-			errorReason = internalerrors.FailedToParseFSGroup
-			errStr := fmt.Sprintf("failed to rotate objects for pod %s/%s, invalid FSGroup:%s", spcps.Namespace, spcps.Status.PodName, spcps.Status.FSGroup)
-			r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("%s, err: %v", errStr, err))
-			return fmt.Errorf("%s, err: %w", errStr, err)
-		}
+	gid, err := fileutil.ParseFSGroup(spcps.Status.FSGroup)
+	if err != nil {
+		errorReason = internalerrors.FailedToParseFSGroup
+		errStr := fmt.Sprintf("failed to rotate objects for pod %s/%s, invalid FSGroup:%s", spcps.Namespace, spcps.Status.PodName, spcps.Status.FSGroup)
+		r.generateEvent(pod, corev1.EventTypeWarning, mountRotationFailedReason, fmt.Sprintf("%s, err: %v", errStr, err))
+		return fmt.Errorf("%s, err: %w", errStr, err)
 	}
 	klog.V(5).InfoS("updating the secret content", "pod", klog.ObjectRef{Namespace: spcps.Namespace, Name: spcps.Status.PodName}, "FSGroup", gid)
 	newObjectVersions, errorReason, err := secretsstore.MountContent(ctx, providerClient, string(paramsJSON), string(secretsJSON), spcps.Status.TargetPath, string(permissionJSON), oldObjectVersions, gid)

pkg/secrets-store/nodeserver.go

@@ -23,10 +23,8 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strconv"
 	"time"
 
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 	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/util/fileutil"
@@ -144,19 +142,12 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 
 	klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags, "volumeCapabilities", req.VolumeCapability.String())
 
-	gid := constants.NoGID // Group ID to Chown the volume contents to
+	// Group ID to Chown the volume contents to
 	mountVol := req.VolumeCapability.GetMount()
-	if len(mountVol.GetVolumeMountGroup()) > 0 {
-		fsGroupStr := mountVol.GetVolumeMountGroup()
-		klog.V(5).Info("fsGroupStr: %v\n", fsGroupStr)
-		gid, err = strconv.ParseInt(fsGroupStr, 10, 64)
-		klog.V(5).Info("converted gid: %v\n", gid)
-		if err != nil {
-			klog.ErrorS(err, "failed to mount secrets store object content", "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName}, "FSGroup: ", fsGroupStr)
-			return nil, status.Error(codes.InvalidArgument, "Error parsing FSGroup")
-		}
-	} else {
-		klog.V(5).InfoS("mount group not set", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName})
+	gid, err := fileutil.ParseFSGroup(mountVol.GetVolumeMountGroup())
+	if err != nil {
+		klog.ErrorS(err, "failed to mount secrets store object content due to invalid FSGroup", "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName}, "fsGroup", mountVol.GetVolumeMountGroup())
+		return nil, status.Errorf(codes.InvalidArgument, "error parsing FSGroup: %v", err)
 	}
 
 	if isMockProvider(providerName) {

pkg/secrets-store/utils.go

@@ -105,7 +105,6 @@ func createOrUpdateSecretProviderClassPodStatus(ctx context.Context, c client.Cl
 	if gid != constants.NoGID {
 		fsGroup = strconv.FormatInt(gid, 10)
 	}
-	klog.V(5).InfoS("setting gid in spcps", "pod", klog.ObjectRef{Namespace: namespace, Name: podname}, "gid", gid, "gidStr", fsGroup)
 	spcPodStatus := &secretsstorev1.SecretProviderClassPodStatus{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      spcpsName,

pkg/util/fileutil/filesystem.go

@@ -21,7 +21,10 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
+
+	"sigs.k8s.io/secrets-store-csi-driver/pkg/constants"
 )
 
 var (
@@ -127,3 +130,15 @@ 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) {
+	if len(fsGroupStr) == 0 {
+		return constants.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
+}

pkg/util/fileutil/filesystem_test.go

@@ -24,6 +24,7 @@ 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) {
@@ -333,3 +334,72 @@ func TestGetVolumeNameFromTargetPath(t *testing.T) {
 		}
 	}
 }
+
+func TestParseFSGroup(t *testing.T) {
+	cases := []struct {
+		name        string
+		fsGroupStr  string
+		want        int64
+		expectedErr bool
+	}{
+		{
+			name:        "empty string returns NoGID",
+			fsGroupStr:  "",
+			want:        constants.NoGID,
+			expectedErr: false,
+		},
+		{
+			name:        "valid gid",
+			fsGroupStr:  "1000",
+			want:        1000,
+			expectedErr: false,
+		},
+		{
+			name:        "valid gid zero",
+			fsGroupStr:  "0",
+			want:        0,
+			expectedErr: false,
+		},
+		{
+			name:        "valid large gid",
+			fsGroupStr:  "65534",
+			want:        65534,
+			expectedErr: false,
+		},
+		{
+			name:        "invalid gid - non-numeric",
+			fsGroupStr:  "INVALID",
+			expectedErr: true,
+		},
+		{
+			name:        "invalid gid - float",
+			fsGroupStr:  "123.45",
+			expectedErr: true,
+		},
+		{
+			name:        "invalid gid - with spaces",
+			fsGroupStr:  "123 456",
+			expectedErr: true,
+		},
+		{
+			name:        "negative gid",
+			fsGroupStr:  "-23",
+			expectedErr: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := ParseFSGroup(tc.fsGroupStr)
+			if tc.expectedErr {
+				if err == nil {
+					t.Errorf("ParseFSGroup(%q) expected error but got none", tc.fsGroupStr)
+				}
+			} else if err != nil {
+				t.Errorf("ParseFSGroup(%q) unexpected error: %v", tc.fsGroupStr, err)
+			} else if got != tc.want {
+				t.Errorf("ParseFSGroup(%q) = %v, want %v", tc.fsGroupStr, got, tc.want)
+			}
+		})
+	}
+}