From 8c5af2b243c2e2d3ccb8db7f1cf706e1e2bf1f2b Mon Sep 17 00:00:00 2001 From: OS-gustavosalvador Date: Thu, 23 Apr 2026 16:13:20 +0100 Subject: [PATCH 1/4] fix: treat empty existing mount as stale in ensureMountPoint On Linux, when the target is already mounted, check whether the directory is empty and unmount it if so, so the caller performs a fresh mount. Matches the contract the Windows branch already enforces and prevents the retry from reporting success on a stale empty tmpfs left by an interrupted NodePublishVolume. Signed-off-by: Gustavo Salvador --- pkg/secrets-store/utils.go | 13 ++++- pkg/secrets-store/utils_test.go | 96 +++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/pkg/secrets-store/utils.go b/pkg/secrets-store/utils.go index af3799785..58ce91e88 100644 --- a/pkg/secrets-store/utils.go +++ b/pkg/secrets-store/utils.go @@ -43,8 +43,19 @@ func (ns *nodeServer) ensureMountPoint(target string) (bool, error) { if !notMnt { // testing original mount point, make sure the mount link is valid - _, err := os.ReadDir(target) + entries, err := os.ReadDir(target) if err == nil { + // Empty existing mount means a previous NodePublishVolume was + // interrupted before writing files. Unmount so the caller + // performs a fresh mount. + if len(entries) == 0 { + klog.InfoS("mount point exists but is empty; unmounting stale mount so caller can perform a fresh mount", "targetPath", target) + if err := ns.mounter.Unmount(target); err != nil { + klog.ErrorS(err, "failed to unmount stale empty mount", "targetPath", target) + return !notMnt, err + } + return false, nil + } klog.InfoS("already mounted to target", "targetPath", target) // already mounted return !notMnt, nil diff --git a/pkg/secrets-store/utils_test.go b/pkg/secrets-store/utils_test.go index 212809ef4..3d060d092 100644 --- a/pkg/secrets-store/utils_test.go +++ b/pkg/secrets-store/utils_test.go @@ -19,6 +19,8 @@ package secretsstore import ( "context" "fmt" + "os" + "path/filepath" "reflect" "testing" @@ -28,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + mount "k8s.io/mount-utils" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -69,6 +72,99 @@ func newSecretProviderClassPodStatus(name, namespace, node string) *secretsstore } } +func TestEnsureMountPoint(t *testing.T) { + // newMountedFakeMounter returns a FakeMounter with target pre-registered + // as a tmpfs mount point, matching what nodeserver.go creates before the + // provider writes content. + newMountedFakeMounter := func(target string) *mount.FakeMounter { + return mount.NewFakeMounter([]mount.MountPoint{{Device: "tmpfs", Path: target, Type: "tmpfs"}}) + } + + tests := []struct { + name string + // setup prepares the target directory (a freshly created tempdir) and + // returns the mounter to use for this case. + setup func(t *testing.T, target string) mount.Interface + // expected return values of ensureMountPoint. + wantMounted bool + wantErr bool + // wantStillMounted is the expected mount state AFTER ensureMountPoint + // runs, asserted via IsLikelyNotMountPoint. + wantStillMounted bool + }{ + { + name: "target is not mounted", + setup: func(t *testing.T, target string) mount.Interface { + return mount.NewFakeMounter([]mount.MountPoint{}) + }, + wantMounted: false, + wantErr: false, + wantStillMounted: false, + }, + { + name: "target is mounted and non-empty", + setup: func(t *testing.T, target string) mount.Interface { + if err := os.WriteFile(filepath.Join(target, "secret1"), []byte("v"), 0644); err != nil { + t.Fatalf("seed: %v", err) + } + return newMountedFakeMounter(target) + }, + wantMounted: true, + wantErr: false, + wantStillMounted: true, + }, + { + // Previous NodePublishVolume was interrupted before writing + // files: the mount persists but the directory is empty. The fix + // unmounts and returns mounted=false so the caller re-mounts. + name: "target is mounted but empty — stale mount is unmounted", + setup: func(t *testing.T, target string) mount.Interface { + return newMountedFakeMounter(target) + }, + wantMounted: false, + wantErr: false, + wantStillMounted: false, + }, + { + name: "target is mounted but empty and unmount fails — error is propagated", + setup: func(t *testing.T, target string) mount.Interface { + m := newMountedFakeMounter(target) + m.UnmountFunc = func(path string) error { + return fmt.Errorf("simulated unmount failure") + } + return m + }, + wantMounted: true, + wantErr: true, + wantStillMounted: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target := t.TempDir() + mounter := tt.setup(t, target) + ns := &nodeServer{mounter: mounter} + + got, err := ns.ensureMountPoint(target) + if (err != nil) != tt.wantErr { + t.Fatalf("ensureMountPoint err = %v, wantErr %v", err, tt.wantErr) + } + if got != tt.wantMounted { + t.Errorf("ensureMountPoint mounted = %v, want %v", got, tt.wantMounted) + } + + notMnt, nmErr := mounter.IsLikelyNotMountPoint(target) + if nmErr != nil { + t.Fatalf("IsLikelyNotMountPoint after: %v", nmErr) + } + if stillMounted := !notMnt; stillMounted != tt.wantStillMounted { + t.Errorf("still mounted after = %v, want %v", stillMounted, tt.wantStillMounted) + } + }) + } +} + func TestCreateOrUpdateSecretProviderClassPodStatus(t *testing.T) { tests := []struct { name string From e9436e04b4084e024d132ae17195de7f3c395694 Mon Sep 17 00:00:00 2001 From: OS-gustavosalvador Date: Wed, 1 Jul 2026 12:21:21 +0100 Subject: [PATCH 2/4] refactor: make ensureMountPoint side-effect-free Signal mount state to the caller instead of unmounting; the caller reuses the existing tmpfs and re-calls the provider. Signed-off-by: Gustavo Salvador --- pkg/secrets-store/nodeserver.go | 10 +++--- pkg/secrets-store/utils.go | 58 ++++++++++++--------------------- pkg/secrets-store/utils_test.go | 53 ++++++++++-------------------- 3 files changed, 44 insertions(+), 77 deletions(-) diff --git a/pkg/secrets-store/nodeserver.go b/pkg/secrets-store/nodeserver.go index 2f09872f3..89bdef4e5 100644 --- a/pkg/secrets-store/nodeserver.go +++ b/pkg/secrets-store/nodeserver.go @@ -71,7 +71,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis var providerName string var podName, podNamespace, podUID string var targetPath string - var mounted, isRemountRequest, skipped, isErrorMasked bool + var mounted, hasContent, isRemountRequest, skipped, isErrorMasked bool errorReason := internalerrors.FailedToMount rotationEnabled := ns.rotationConfig.enabled @@ -137,7 +137,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } } - mounted, err = ns.ensureMountPoint(targetPath) + mounted, hasContent, err = ns.ensureMountPoint(targetPath) if err != nil { // kubelet will not create the CSI NodePublishVolume target directory in 1.20+, in accordance with the CSI specification. // CSI driver needs to properly create and process the target path @@ -150,11 +150,11 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis return nil, status.Errorf(codes.Internal, "failed to check if target path %s is mount point, err: %v", targetPath, err) } } - // If it is mounted, it means this is not the first time mount request for this path. - isRemountRequest = mounted + // An empty existing mount is a previous interrupted call, not a remount. + isRemountRequest = mounted && hasContent // If rotation is not enabled, don't remount the already mounted secrets. - if !rotationEnabled && mounted { + if !rotationEnabled && isRemountRequest { klog.InfoS("target path is already mounted", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName}) skipped = true return &csi.NodePublishVolumeResponse{}, nil diff --git a/pkg/secrets-store/utils.go b/pkg/secrets-store/utils.go index 58ce91e88..f4ecc31cf 100644 --- a/pkg/secrets-store/utils.go +++ b/pkg/secrets-store/utils.go @@ -34,57 +34,41 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// ensureMountPoint ensures mount point is valid -func (ns *nodeServer) ensureMountPoint(target string) (bool, error) { +// ensureMountPoint reports whether the target is mounted and whether +// it has any content. The caller decides whether to mount, populate, +// or short-circuit. +func (ns *nodeServer) ensureMountPoint(target string) (mounted, hasContent bool, err error) { notMnt, err := ns.mounter.IsLikelyNotMountPoint(target) if err != nil { - return !notMnt, err + return false, false, err } if !notMnt { - // testing original mount point, make sure the mount link is valid - entries, err := os.ReadDir(target) - if err == nil { - // Empty existing mount means a previous NodePublishVolume was - // interrupted before writing files. Unmount so the caller - // performs a fresh mount. - if len(entries) == 0 { - klog.InfoS("mount point exists but is empty; unmounting stale mount so caller can perform a fresh mount", "targetPath", target) - if err := ns.mounter.Unmount(target); err != nil { - klog.ErrorS(err, "failed to unmount stale empty mount", "targetPath", target) - return !notMnt, err - } - return false, nil - } - klog.InfoS("already mounted to target", "targetPath", target) - // already mounted - return !notMnt, nil + entries, readErr := os.ReadDir(target) + if readErr == nil { + return true, len(entries) > 0, nil } - if err := ns.mounter.Unmount(target); err != nil { - klog.ErrorS(err, "failed to unmount directory", "targetPath", target) - return !notMnt, err + // Mount is in a bad state. Unmount and let the caller perform a + // fresh mount. + if unmountErr := ns.mounter.Unmount(target); unmountErr != nil { + klog.ErrorS(unmountErr, "failed to unmount directory", "targetPath", target) + return true, false, unmountErr } - notMnt = true - // remount it in node publish - return !notMnt, err + return false, false, nil } if runtimeutil.IsRuntimeWindows() { - // IsLikelyNotMountPoint always returns notMnt=true for windows as the - // target path is not a soft link to the global mount - // instead check if the dir exists for windows and if it's not empty - // If there are contents in the dir, then objects are already mounted - f, err := os.ReadDir(target) + // IsLikelyNotMountPoint always returns notMnt=true on Windows; + // use directory content as the "is mounted" proxy. + entries, err := os.ReadDir(target) if err != nil { - return !notMnt, err - } - if len(f) > 0 { - notMnt = false - return !notMnt, err + return false, false, err } + present := len(entries) > 0 + return present, present, nil } - return false, nil + return false, false, nil } func (ns *nodeServer) getLastUpdateTime(target string) (time.Time, error) { diff --git a/pkg/secrets-store/utils_test.go b/pkg/secrets-store/utils_test.go index 3d060d092..2e63ded7a 100644 --- a/pkg/secrets-store/utils_test.go +++ b/pkg/secrets-store/utils_test.go @@ -81,15 +81,11 @@ func TestEnsureMountPoint(t *testing.T) { } tests := []struct { - name string - // setup prepares the target directory (a freshly created tempdir) and - // returns the mounter to use for this case. - setup func(t *testing.T, target string) mount.Interface - // expected return values of ensureMountPoint. - wantMounted bool - wantErr bool - // wantStillMounted is the expected mount state AFTER ensureMountPoint - // runs, asserted via IsLikelyNotMountPoint. + name string + setup func(t *testing.T, target string) mount.Interface + wantMounted bool + wantHasContent bool + wantErr bool wantStillMounted bool }{ { @@ -97,12 +93,9 @@ func TestEnsureMountPoint(t *testing.T) { setup: func(t *testing.T, target string) mount.Interface { return mount.NewFakeMounter([]mount.MountPoint{}) }, - wantMounted: false, - wantErr: false, - wantStillMounted: false, }, { - name: "target is mounted and non-empty", + name: "target is mounted and populated", setup: func(t *testing.T, target string) mount.Interface { if err := os.WriteFile(filepath.Join(target, "secret1"), []byte("v"), 0644); err != nil { t.Fatalf("seed: %v", err) @@ -110,32 +103,19 @@ func TestEnsureMountPoint(t *testing.T) { return newMountedFakeMounter(target) }, wantMounted: true, - wantErr: false, + wantHasContent: true, wantStillMounted: true, }, { - // Previous NodePublishVolume was interrupted before writing - // files: the mount persists but the directory is empty. The fix - // unmounts and returns mounted=false so the caller re-mounts. - name: "target is mounted but empty — stale mount is unmounted", + // A previous NodePublishVolume was interrupted before writing + // files. The mount is reported as existing but empty; the caller + // re-calls the provider to populate it. + name: "target is mounted but empty", setup: func(t *testing.T, target string) mount.Interface { return newMountedFakeMounter(target) }, - wantMounted: false, - wantErr: false, - wantStillMounted: false, - }, - { - name: "target is mounted but empty and unmount fails — error is propagated", - setup: func(t *testing.T, target string) mount.Interface { - m := newMountedFakeMounter(target) - m.UnmountFunc = func(path string) error { - return fmt.Errorf("simulated unmount failure") - } - return m - }, wantMounted: true, - wantErr: true, + wantHasContent: false, wantStillMounted: true, }, } @@ -146,12 +126,15 @@ func TestEnsureMountPoint(t *testing.T) { mounter := tt.setup(t, target) ns := &nodeServer{mounter: mounter} - got, err := ns.ensureMountPoint(target) + mounted, hasContent, err := ns.ensureMountPoint(target) if (err != nil) != tt.wantErr { t.Fatalf("ensureMountPoint err = %v, wantErr %v", err, tt.wantErr) } - if got != tt.wantMounted { - t.Errorf("ensureMountPoint mounted = %v, want %v", got, tt.wantMounted) + if mounted != tt.wantMounted { + t.Errorf("mounted = %v, want %v", mounted, tt.wantMounted) + } + if hasContent != tt.wantHasContent { + t.Errorf("hasContent = %v, want %v", hasContent, tt.wantHasContent) } notMnt, nmErr := mounter.IsLikelyNotMountPoint(target) From 539013c2cf903ed9b39cbf7a747ad5249a6a0b1c Mon Sep 17 00:00:00 2001 From: OS-gustavosalvador Date: Wed, 1 Jul 2026 12:29:40 +0100 Subject: [PATCH 3/4] fix lint: use 0600 file mode in TestEnsureMountPoint Signed-off-by: OS-gustavosalvador --- pkg/secrets-store/utils_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/secrets-store/utils_test.go b/pkg/secrets-store/utils_test.go index 2e63ded7a..da9d2b606 100644 --- a/pkg/secrets-store/utils_test.go +++ b/pkg/secrets-store/utils_test.go @@ -97,7 +97,7 @@ func TestEnsureMountPoint(t *testing.T) { { name: "target is mounted and populated", setup: func(t *testing.T, target string) mount.Interface { - if err := os.WriteFile(filepath.Join(target, "secret1"), []byte("v"), 0644); err != nil { + if err := os.WriteFile(filepath.Join(target, "secret1"), []byte("v"), 0600); err != nil { t.Fatalf("seed: %v", err) } return newMountedFakeMounter(target) From fbc5bf3eb896d86957f085184be7ec84fbc0ce6f Mon Sep 17 00:00:00 2001 From: OS-gustavosalvador Date: Thu, 20 Aug 2026 16:02:27 +0100 Subject: [PATCH 4/4] fix: review comments Signed-off-by: OS-gustavosalvador --- pkg/secrets-store/nodeserver.go | 8 ++++---- pkg/secrets-store/utils.go | 10 ++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/secrets-store/nodeserver.go b/pkg/secrets-store/nodeserver.go index 89bdef4e5..b603a066a 100644 --- a/pkg/secrets-store/nodeserver.go +++ b/pkg/secrets-store/nodeserver.go @@ -150,12 +150,12 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis return nil, status.Errorf(codes.Internal, "failed to check if target path %s is mount point, err: %v", targetPath, err) } } - // An empty existing mount is a previous interrupted call, not a remount. - isRemountRequest = mounted && hasContent + // If it is mounted, it means this is not the first time mount request for this path. + isRemountRequest = mounted // If rotation is not enabled, don't remount the already mounted secrets. - if !rotationEnabled && isRemountRequest { - klog.InfoS("target path is already mounted", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName}) + if !rotationEnabled && mounted && hasContent { + klog.InfoS("target path is already mounted and populated", "targetPath", targetPath, "pod", klog.ObjectRef{Namespace: podNamespace, Name: podName}) skipped = true return &csi.NodePublishVolumeResponse{}, nil } diff --git a/pkg/secrets-store/utils.go b/pkg/secrets-store/utils.go index f4ecc31cf..d2a18cd82 100644 --- a/pkg/secrets-store/utils.go +++ b/pkg/secrets-store/utils.go @@ -35,8 +35,7 @@ import ( ) // ensureMountPoint reports whether the target is mounted and whether -// it has any content. The caller decides whether to mount, populate, -// or short-circuit. +// it has any content. func (ns *nodeServer) ensureMountPoint(target string) (mounted, hasContent bool, err error) { notMnt, err := ns.mounter.IsLikelyNotMountPoint(target) if err != nil { @@ -46,6 +45,7 @@ func (ns *nodeServer) ensureMountPoint(target string) (mounted, hasContent bool, if !notMnt { entries, readErr := os.ReadDir(target) if readErr == nil { + klog.V(4).InfoS("already mounted to target", "targetPath", target) return true, len(entries) > 0, nil } // Mount is in a bad state. Unmount and let the caller perform a @@ -58,8 +58,10 @@ func (ns *nodeServer) ensureMountPoint(target string) (mounted, hasContent bool, } if runtimeutil.IsRuntimeWindows() { - // IsLikelyNotMountPoint always returns notMnt=true on Windows; - // use directory content as the "is mounted" proxy. + // IsLikelyNotMountPoint always returns notMnt=true for windows as the + // target path is not a soft link to the global mount + // instead check if the dir exists for windows and if it's not empty + // If there are contents in the dir, then objects are already mounted entries, err := os.ReadDir(target) if err != nil { return false, false, err