k8s-device-plugin icon indicating copy to clipboard operation
k8s-device-plugin copied to clipboard

[gfd] Fixing logic of atomically file writing

Open belo4ya opened this issue 1 year ago • 3 comments

fix #791

The current implementation of the writeFileAtomically function contains a bug - the os.Chmod function is called after os.Rename, causing a moment when the target file (features.d/gfd) has incorrect permissions (0600 instead of 0644):

E0629 17:29:25.340196       1 local.go:266] source local failed reading file 'gfd': open /etc/kubernetes/node-feature-discovery/features.d/gfd: permission denied

To fix this issue, the order of the calls needs to be changed to: os.Chmod -> os.Rename

belo4ya avatar Jun 29 '24 23:06 belo4ya

What do you think about using github.com/google/renameio instead of our own implementation of atomic file writing?

import "github.com/google/renameio"

func writeFileAtomically(path string, contents []byte, perm os.FileMode) error {
	return renameio.WriteFile(path, contents, perm)
}

github.com/google/renameio is stable, has no dependencies, is quite popular (used by 4.3k), and performs its task well.

belo4ya avatar Jun 29 '24 23:06 belo4ya

@belo4ya thanks for the fix. I think we would be ok with using renameio. Feel free to update this PR to switch to that instead. Alternatively we can do so as a follow-up.

elezar avatar Jul 03 '24 14:07 elezar

Done. I replaced the function writeFileAtomically with renameio.WriteFile.

The only thing is, the temporary file will be created in $TMPDIR instead of /etc/kubernetes/node-feature-discovery/features.d/gfd-tmp/ (I don't think this is a problem, but it's worth mentioning).

belo4ya avatar Jul 03 '24 15:07 belo4ya

@elezar, done.

belo4ya avatar Jul 10 '24 06:07 belo4ya