Skip to content

Commit fe04017

Browse files
committed
Address review comments (round 2)
- Update function comment to accurately describe symlink reuse behavior - Only retry rename on os.IsExist; fail fast on permission/IO errors - Update test comment to reflect filesystem-based concurrency (no CRUID lock)
1 parent 1a9117b commit fe04017

2 files changed

Lines changed: 12 additions & 3 deletions

File tree

pkg/smb/nodeserver.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,8 @@ func getKerberosCache(krb5CacheDirectory, krb5Prefix string, credUID int, secret
585585
// Create kerberos cache in the file based on the VolumeID, so it can be cleaned up during unstage
586586
// At the same time, kerberos expects to find cache in file named "krb5cc_*", so creating symlink
587587
// will allow both clean up and serving proper cache to the kerberos.
588-
// If symlink already exists, ignore it.
588+
// If a valid symlink already exists (pointing to a readable cache file), it is kept as-is
589+
// to avoid disrupting concurrent mounts. Dangling or non-symlink entries are replaced.
589590
func (d *Driver) ensureKerberosCache(krb5CacheDirectory, krb5Prefix, volumeID string, mountFlags []string, secrets map[string]string) (bool, error) {
590591
if !hasKerberosMountOption(mountFlags) {
591592
return false, nil
@@ -660,6 +661,14 @@ func (d *Driver) ensureKerberosCache(krb5CacheDirectory, krb5Prefix, volumeID st
660661
}
661662

662663
if err := os.Rename(tempSymlinkName, krb5CacheFileName); err != nil {
664+
// On Linux, Rename replaces the target atomically; this branch is
665+
// only expected on systems where rename-over-existing fails (e.g.,
666+
// certain Windows/NFS edge cases). Only retry when the target
667+
// already exists; for other errors (permission, I/O) fail fast.
668+
if !os.IsExist(err) {
669+
os.Remove(tempSymlinkName)
670+
return false, status.Error(codes.Internal, fmt.Sprintf("Couldn't atomically update kerberos symlink %s -> %s for credUID %d: %v", krb5CacheFileName, volumeIDCacheAbsolutePath, credUID, err))
671+
}
663672
if removeErr := os.Remove(krb5CacheFileName); removeErr != nil && !os.IsNotExist(removeErr) {
664673
os.Remove(tempSymlinkName)
665674
return false, status.Error(codes.Internal, fmt.Sprintf("Couldn't replace kerberos symlink %s: rename failed: %v, remove failed: %v", krb5CacheFileName, err, removeErr))

pkg/smb/nodeserver_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1065,8 +1065,8 @@ func TestEnsureKerberosCache(t *testing.T) {
10651065
}
10661066

10671067
// Regression test for the race where parallel kubelet mounts sharing a CRUID
1068-
// collided on the shared krb5cc_<uid> symlink. Runs many goroutines against the
1069-
// per-CRUID lock and asserts no caller observes a "file exists" from Symlink
1068+
// collided on the shared krb5cc_<uid> symlink. Runs many goroutines concurrently
1069+
// and asserts that atomic temp-symlink-then-rename avoids "file exists" errors
10701070
// and that the final symlink points at a valid volume-specific cache file.
10711071
func TestEnsureKerberosCacheConcurrent(t *testing.T) {
10721072
if runtime.GOOS != "linux" {

0 commit comments

Comments
 (0)