Address review comments by

1. pkg/secrets-store/nodeserver.go
   - Organize the imports as stdlib, internal, external
2. pkg/secrets-store/nodeserver_test.go
   -  Create a deep empty VolumeCapability field in the NodePublishRequest template
3. pkg/util/fileutil/atomic_writer.go
   - Add comment in the header to explain the change of data type from int64 to int for FSGroup.
   - Reason for changing FSUser to FSGroup was already mentioned in the header
4. pkg/util/fileutil/filesystem.go and filesystem_test.go
   - disallow negative FSGroup.
   - ( Earlier, the intention was to keep complete type compatibility, and let the implementation (os.Chown) handle the full range that it supports.
       but as fsGroup is validated at API to be in range: 0 to 2147483647, disallowing the negative values)
5. test/bats/e2e-provider.bats
   - Fix delete_pod() to check the driver logs for unmount errors instead of the created pod logs
   - Fix enable_secret_rotation() to error out on kubectl errors

Author: mytreya-rh

pkg/secrets-store/nodeserver.go

@@ -26,14 +26,15 @@ import (
 	"path/filepath"
 	"time"
 
+	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
+	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/fileutil"
+
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"k8s.io/klog/v2"
 	mount "k8s.io/mount-utils"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors"
-	"sigs.k8s.io/secrets-store-csi-driver/pkg/util/fileutil"
 )
 
 type nodeServer struct {

pkg/secrets-store/nodeserver_test.go

@@ -77,8 +77,12 @@ func getInitObjects(customize func(*secretsstorev1.SecretProviderClass)) []clien
 
 func getRequest(t *testing.T, customize func(*csi.NodePublishVolumeRequest)) *csi.NodePublishVolumeRequest {
 	var request = &csi.NodePublishVolumeRequest{
-		VolumeCapability: &csi.VolumeCapability{},
-		VolumeId:         "testvolid1",
+		VolumeCapability: &csi.VolumeCapability{
+			AccessType: &csi.VolumeCapability_Mount{
+				Mount: &csi.VolumeCapability_MountVolume{},
+			},
+		},
+		VolumeId: "testvolid1",
 		VolumeContext: map[string]string{
 			"secretProviderClass": "provider1",
 			csiPodName:            "pod1",

pkg/util/fileutil/atomic_writer.go

@@ -20,6 +20,7 @@ limitations under the License.
 //  * link: https://github.com/kubernetes/kubernetes/blob/8a62859e515889f07e3e3be6a1080413f17cf2c3/pkg/volume/util/atomic_writer.go
 // In addition, FileProjection::FSUser has been changed to FileProjection::FSGroup
 //    to facilitate support for FSGroup csi.NodeServiceCapability_RPC_VOLUME_MOUNT_GROUP
+//    and the type changed to int to match directly with the os.Chown() function arguments.
 
 package fileutil
 

pkg/util/fileutil/filesystem.go

@@ -18,6 +18,7 @@ limitations under the License.
 package fileutil
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -140,5 +141,12 @@ func ParseFSGroup(fsGroupStr string) (int, error) {
 	if len(fsGroupStr) == 0 {
 		return NoGID, nil
 	}
-	return strconv.Atoi(fsGroupStr)
+	gid, err := strconv.Atoi(fsGroupStr)
+	if err != nil {
+		return NoGID, err
+	}
+	if gid < 0 {
+		return NoGID, fmt.Errorf("invalid FSGroup: %d must be non-negative", gid)
+	}
+	return gid, nil
 }

pkg/util/fileutil/filesystem_test.go

@@ -381,9 +381,9 @@ func TestParseFSGroup(t *testing.T) {
 			expectedErr: true,
 		},
 		{
-			name:       "negative gid",
-			fsGroupStr: "-23",
-			want:       -23,
+			name:        "negative gid",
+			fsGroupStr:  "-23",
+			expectedErr: true,
 		},
 	}
 

test/bats/e2e-provider.bats

@@ -98,17 +98,17 @@ 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 | grep '^E.*failed to clean and unmount target path.*$'"
+  run bash -c "kubectl -n $NAMESPACE logs -l app=secrets-store-csi-driver --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 > /dev/null
-  kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null
+  kubectl run ${curl_pod_name} -n rotation --image=curlimages/curl:7.75.0 --labels="test=rotation" -- tail -f /dev/null > /dev/null || return 1
+  kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null || return 1
   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
+  kubectl exec ${curl_pod_name} -n rotation -- curl http://${pod_ip}:8080/rotation?rotated=true > /dev/null || return 1
   # wait for rotated secret to be mounted
   sleep 120
   echo "${curl_pod_name}"