Skip to content

Commit de8cc58

Browse files
fix: treat empty existing mount as stale in ensureMountPoint
On Linux, ensureMountPoint returned mounted=true for any existing mount whose directory could be listed, without checking whether the mount actually had content. If a previous NodePublishVolume was interrupted between mounting the tmpfs and writing provider objects to it (driver pod killed, context canceled), the kernel-level mount persisted but was empty. The next NodePublishVolume retry short-circuited at the "target path is already mounted" branch and reported success on an empty directory, causing the pod to start with no secrets. This preserves the same invariant the Windows branch of the function already enforces - "empty target directory means the caller should perform a fresh mount" - now correctly on Linux where a stale tmpfs can persist across driver pod restarts. Signed-off-by: Gustavo Salvador <gustavo.salvador@outsystems.com>
1 parent ae2b761 commit de8cc58

2 files changed

Lines changed: 116 additions & 1 deletion

File tree

pkg/secrets-store/utils.go

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,24 @@ func (ns *nodeServer) ensureMountPoint(target string) (bool, error) {
4343

4444
if !notMnt {
4545
// testing original mount point, make sure the mount link is valid
46-
_, err := os.ReadDir(target)
46+
entries, err := os.ReadDir(target)
4747
if err == nil {
48+
// A mount that exists but has no content indicates that a previous
49+
// NodePublishVolume call was interrupted (e.g. the driver pod was
50+
// killed, or the gRPC context was canceled) between mounting the
51+
// tmpfs and writing provider objects to it. Returning mounted=true
52+
// here would cause NodePublishVolume to short-circuit on the next
53+
// retry and report success for an empty directory, starting the
54+
// pod with no secrets. Unmount so the caller performs a fresh
55+
// mount.
56+
if len(entries) == 0 {
57+
klog.InfoS("mount point exists but is empty; unmounting stale mount so caller can perform a fresh mount", "targetPath", target)
58+
if err := ns.mounter.Unmount(target); err != nil {
59+
klog.ErrorS(err, "failed to unmount stale empty mount", "targetPath", target)
60+
return !notMnt, err
61+
}
62+
return false, nil
63+
}
4864
klog.InfoS("already mounted to target", "targetPath", target)
4965
// already mounted
5066
return !notMnt, nil

pkg/secrets-store/utils_test.go

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ package secretsstore
1919
import (
2020
"context"
2121
"fmt"
22+
"os"
23+
"path/filepath"
2224
"reflect"
2325
"testing"
2426

@@ -28,6 +30,7 @@ import (
2830
"k8s.io/apimachinery/pkg/runtime"
2931
"k8s.io/apimachinery/pkg/types"
3032
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
33+
mount "k8s.io/mount-utils"
3134
"sigs.k8s.io/controller-runtime/pkg/client"
3235
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3336
)
@@ -69,6 +72,102 @@ func newSecretProviderClassPodStatus(name, namespace, node string) *secretsstore
6972
}
7073
}
7174

75+
func TestEnsureMountPoint(t *testing.T) {
76+
// newMountedFakeMounter returns a FakeMounter with target pre-registered
77+
// as a tmpfs mount point, matching what nodeserver.go creates before the
78+
// provider writes content.
79+
newMountedFakeMounter := func(target string) *mount.FakeMounter {
80+
return mount.NewFakeMounter([]mount.MountPoint{{Device: "tmpfs", Path: target, Type: "tmpfs"}})
81+
}
82+
83+
tests := []struct {
84+
name string
85+
// setup prepares the target directory (a freshly created tempdir) and
86+
// returns the mounter to use for this case.
87+
setup func(t *testing.T, target string) mount.Interface
88+
// expected return values of ensureMountPoint.
89+
wantMounted bool
90+
wantErr bool
91+
// wantStillMounted is the expected mount state AFTER ensureMountPoint
92+
// runs, asserted via IsLikelyNotMountPoint.
93+
wantStillMounted bool
94+
}{
95+
{
96+
name: "target is not mounted",
97+
setup: func(t *testing.T, target string) mount.Interface {
98+
return mount.NewFakeMounter([]mount.MountPoint{})
99+
},
100+
wantMounted: false,
101+
wantErr: false,
102+
wantStillMounted: false,
103+
},
104+
{
105+
name: "target is mounted and non-empty",
106+
setup: func(t *testing.T, target string) mount.Interface {
107+
if err := os.WriteFile(filepath.Join(target, "secret1"), []byte("v"), 0644); err != nil {
108+
t.Fatalf("seed: %v", err)
109+
}
110+
return newMountedFakeMounter(target)
111+
},
112+
wantMounted: true,
113+
wantErr: false,
114+
wantStillMounted: true,
115+
},
116+
{
117+
// Regression for the #1051 race: a previous NodePublishVolume call
118+
// was interrupted between mounting tmpfs and writing content, so
119+
// the kernel mount persists but the directory is empty. The fix
120+
// unmounts the stale mount and returns mounted=false so the caller
121+
// performs a fresh mount instead of short-circuiting to success on
122+
// an empty directory.
123+
name: "target is mounted but empty — stale mount is unmounted",
124+
setup: func(t *testing.T, target string) mount.Interface {
125+
return newMountedFakeMounter(target)
126+
},
127+
wantMounted: false,
128+
wantErr: false,
129+
wantStillMounted: false,
130+
},
131+
{
132+
name: "target is mounted but empty and unmount fails — error is propagated",
133+
setup: func(t *testing.T, target string) mount.Interface {
134+
m := newMountedFakeMounter(target)
135+
m.UnmountFunc = func(path string) error {
136+
return fmt.Errorf("simulated unmount failure")
137+
}
138+
return m
139+
},
140+
wantMounted: true,
141+
wantErr: true,
142+
wantStillMounted: true,
143+
},
144+
}
145+
146+
for _, tt := range tests {
147+
t.Run(tt.name, func(t *testing.T) {
148+
target := t.TempDir()
149+
mounter := tt.setup(t, target)
150+
ns := &nodeServer{mounter: mounter}
151+
152+
got, err := ns.ensureMountPoint(target)
153+
if (err != nil) != tt.wantErr {
154+
t.Fatalf("ensureMountPoint err = %v, wantErr %v", err, tt.wantErr)
155+
}
156+
if got != tt.wantMounted {
157+
t.Errorf("ensureMountPoint mounted = %v, want %v", got, tt.wantMounted)
158+
}
159+
160+
notMnt, nmErr := mounter.IsLikelyNotMountPoint(target)
161+
if nmErr != nil {
162+
t.Fatalf("IsLikelyNotMountPoint after: %v", nmErr)
163+
}
164+
if stillMounted := !notMnt; stillMounted != tt.wantStillMounted {
165+
t.Errorf("still mounted after = %v, want %v", stillMounted, tt.wantStillMounted)
166+
}
167+
})
168+
}
169+
}
170+
72171
func TestCreateOrUpdateSecretProviderClassPodStatus(t *testing.T) {
73172
tests := []struct {
74173
name string

0 commit comments

Comments
 (0)