Fix further code review comments

1. pkg/secrets-store/nodeserver.go
   - update the log to just log VolumeMountGroup instead of VolumeCapabilities
   - Here, not using the parsed value but the value obtained in the NodePublishVolume arguments
     as it could help in better debugging when somehow the parse function is not working as expected
2. pkg/util/fileutil/atomic_writer.go
   - update the comment in FileProjection structure explaining divergence from upstream
   - update the error message on chown failure to reflect that group ownership change is what failed
3. test/bats/e2e-provider.bats
   - correct the namespace while getting logs from the driver
   - simplify the filePermission configuration (e2e_provider_secretproviderclass.yaml)
4. test/e2eprovider/server/server.go
   - make error logging more explicit above case where filePermission is > 0777
   - changed the comparison from 511 to 0o777 for readability

Author: mytreya-rh

pkg/secrets-store/nodeserver.go

@@ -161,10 +161,9 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 		return &csi.NodePublishVolumeResponse{}, nil
 	}
 
-	klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags, "volumeCapabilities", req.VolumeCapability.String())
-
 	// Group ID to Chown the volume contents to
-	mountVol := req.VolumeCapability.GetMount()
+	mountVol := req.GetVolumeCapability().GetMount()
+	klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags, "volumeMountGroup", mountVol.GetVolumeMountGroup())
 	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())

pkg/util/fileutil/atomic_writer.go

@@ -68,7 +68,8 @@ type AtomicWriter struct {
 	logContext string
 }
 
-// FileProjection contains file Data and access Mode
+// FileProjection contains file Data and access Mode.
+// FsGroup diverges from upstream's FsUser (*int64) — see file header for rationale.
 type FileProjection struct {
 	Data    []byte
 	Mode    int32
@@ -417,7 +418,7 @@ func (w *AtomicWriter) writePayloadToDir(payload map[string]FileProjection, dir
 			continue
 		}
 		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)
+			klog.ErrorS(err, "unable to change file group ownership", "logContext", w.logContext, "fullPath", fullPath, "group", *fileProjection.FsGroup)
 			return err
 		}
 	}

test/bats/e2e-provider.bats

@@ -18,7 +18,7 @@ export SECRET_VERSION=${SECRET_VERSION:-"v1"}
 # default secret value returned by the mock provider
 export SECRET_VALUE=${SECRET_VALUE:-"secret"}
 # default secret mode returned by the mock provider
-export SECRET_MODE=${SECRET_MODE:-'"0644"'}
+export SECRET_MODE=${SECRET_MODE:-0644}
 
 # export key vars
 export KEY_NAME=${KEY_NAME:-fookey}
@@ -28,7 +28,7 @@ export KEY_VERSION=${KEY_VERSION:-"v1"}
 # base64 encoded content comparision is easier in case of very long multiline string.
 export KEY_VALUE_CONTAINS=${KEY_VALUE:-"LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KVGhpcyBpcyBtb2NrIGtleQotLS0tLUVORCBQVUJMSUMgS0VZLS0tLS0K"}
 # default version value returned by mock provider
-export KEY_MODE=${KEY_MODE:-'"0644"'}
+export KEY_MODE=${KEY_MODE:-0644}
 
 # export node selector var
 export NODE_SELECTOR_OS=$NODE_SELECTOR_OS
@@ -98,7 +98,7 @@ 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=secrets-store-csi-driver --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'"
+  run bash -c "kubectl -n kube-system logs -l app=secrets-store-csi-driver --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'"
   assert_failure
 }
 
@@ -223,7 +223,7 @@ function disable_secret_rotation() {
 }
 
 @test "deploy e2e-provider v1 secretproviderclass crd with restricted permissions" {
-  SPC_NAME="e2e-provider-640" SECRET_MODE='"0640"' KEY_MODE='"0640"' create_spc
+  SPC_NAME="e2e-provider-640" SECRET_MODE=0640 KEY_MODE=0640 create_spc
 }
 
 @test "Non-root POD with no FSGroup - create" {

test/bats/tests/e2e_provider/e2e_provider_secretproviderclass.yaml

@@ -10,8 +10,8 @@ spec:
         - |
           objectName: $SECRET_NAME
           objectVersion: $SECRET_VERSION
-          filePermission: $SECRET_MODE
+          filePermission: "$SECRET_MODE"
         - |
           objectName: $KEY_NAME
           objectVersion: $KEY_VERSION
-          filePermission: $KEY_MODE
+          filePermission: "$KEY_MODE"

test/e2eprovider/server/server.go

@@ -180,9 +180,12 @@ func (s *Server) Mount(ctx context.Context, req *v1alpha1.MountRequest) (*v1alph
 		klog.InfoS("Secret Object with", "name", mockSecretsStoreObject.ObjectName, "permission", mockSecretsStoreObject.FilePermission)
 		if len(mockSecretsStoreObject.FilePermission) > 0 {
 			mode, err := strconv.ParseUint(mockSecretsStoreObject.FilePermission, 8, 32)
-			if err != nil || mode > 511 {
+			if err != nil {
 				return nil, fmt.Errorf("invalid filePermission: %s, error: %w for file: %s", mockSecretsStoreObject.FilePermission, err, mockSecretsStoreObject.ObjectName)
 			}
+			if mode > 0o777 {
+				return nil, fmt.Errorf("invalid filePermission: %s exceeds 0777 for file: %s", mockSecretsStoreObject.FilePermission, mockSecretsStoreObject.ObjectName)
+			}
 			secretFile.Mode = int32(mode)
 			klog.InfoS("Set file mode", "file", secretFile.Path, "mode", os.FileMode(mode))
 		}