Skip to content

Refactor filesystem.go and adapt tests to use a real file system #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor filesystem.go and adapt tests to use a real file system #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/simple/deploy/01_simple-csi-driver.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ spec:
args :
- --node-id=$(NODE_ID)
- --endpoint=$(CSI_ENDPOINT)
- --data-root=csi-data-dir
- --data-root=/csi-data-dir
env:
- name: NODE_ID
valueFrom:
Expand Down
122 changes: 60 additions & 62 deletions storage/filesystem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ import (
"github.com/cert-manager/csi-lib/third_party/util"
)

// These variables can be overridden by tests, this makes it easier to test
// the filesystem implementation as a non-root user.
var (
parentFolderMode fs.FileMode = 0700
volumeFolderMode fs.FileMode = 0644
volumeDataFolderMode fs.FileMode = 0550
)

const (
readWriteUserFileMode = 0600
readOnlyUserAndGroupFileMode = 0440
Expand All @@ -42,12 +50,9 @@ const (
type Filesystem struct {
log logr.Logger

// baseDir is the absolute path to a directory used to store all metadata
// basePath is the absolute path to a directory used to store all metadata
// about mounted volumes and mount points.
baseDir string

// used by the 'read only' methods
fs fs.StatFS
basePath string

// FSGroupVolumeAttributeKey is an optional well-known key in the volume
// attributes. If this attribute is present in the context when writing
Expand All @@ -59,30 +64,44 @@ type Filesystem struct {
// Ensure the Filesystem implementation is fully featured
var _ Interface = &Filesystem{}

func NewFilesystem(log logr.Logger, baseDir string) (*Filesystem, error) {
f := &Filesystem{
log: log,
baseDir: baseDir,
// Use the rootfs as the DirFS so that paths passed to both read &
// write methods on this struct use a consistent root.
fs: os.DirFS("/").(fs.StatFS),
func NewFilesystem(log logr.Logger, basePath string) (*Filesystem, error) {
inmemfsPath := filepath.Join(basePath, "inmemfs")

f, err := NewFilesystemOnDisk(log, inmemfsPath)
if err != nil {
return nil, err
}

isMnt, err := mount.New("").IsMountPoint(f.tempfsPath())
isMnt, err := mount.New("").IsMountPoint(inmemfsPath)
if err != nil {
if !os.IsNotExist(err) {
return nil, err
}
if err := os.MkdirAll(f.tempfsPath(), 0700); err != nil {
return nil, err
}
return nil, err
}

if !isMnt {
if err := mount.New("").Mount("tmpfs", f.tempfsPath(), "tmpfs", []string{}); err != nil {
if err := mount.New("").Mount("tmpfs", inmemfsPath, "tmpfs", []string{}); err != nil {
return nil, fmt.Errorf("mounting tmpfs: %w", err)
}
log.Info("Mounted new tmpfs", "path", f.tempfsPath())
log.Info("Mounted new tmpfs", "path", inmemfsPath)
}

return f, nil
}

// WARNING: should be used only for testing purposes only, as we don't want to store
// any sensitive data on disk in production.
func NewFilesystemOnDisk(log logr.Logger, basePath string) (*Filesystem, error) {
if !filepath.IsAbs(basePath) {
return nil, fmt.Errorf("baseDir must be an absolute path")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting from this PR, we no longer support the weird relative path notation. All paths have to be absolute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see https://github.com/cert-manager/csi-lib/pull/71/files#diff-ebf7d236278a68704d3f90c6e44983c3c663d2beb9ce124a1658cb93ffaa6276R89

}

f := &Filesystem{
log: log,
basePath: basePath,
}

// Create the base directory if it does not exist.
if err := os.MkdirAll(basePath, parentFolderMode); err != nil {
return nil, err
}

return f, nil
Expand All @@ -93,18 +112,18 @@ func (f *Filesystem) PathForVolume(volumeID string) string {
}

func (f *Filesystem) RemoveVolume(volumeID string) error {
return os.RemoveAll(filepath.Join(f.tempfsPath(), volumeID))
return os.RemoveAll(f.volumePath(volumeID))
}

func (f *Filesystem) ListVolumes() ([]string, error) {
dirs, err := fs.ReadDir(f.fs, f.tempfsPath())
dirs, err := os.ReadDir(f.basePath)
if err != nil {
return nil, fmt.Errorf("listing volumes: %w", err)
}

var vols []string
for _, dir := range dirs {
_, err := f.fs.Stat(f.metadataPathForVolumeID(dir.Name()))
_, err := os.Stat(f.metadataPathForVolumeID(dir.Name()))
switch {
case errors.Is(err, fs.ErrNotExist):
f.log.Info("Directory exists but does not contain a metadata file - deleting directory and its contents", "volume_id", dir.Name())
Expand All @@ -127,7 +146,7 @@ func (f *Filesystem) ListVolumes() ([]string, error) {
// Errors wrapping ErrNotFound will be returned if metadata for the ID cannot
// be found.
func (f *Filesystem) ReadMetadata(volumeID string) (metadata.Metadata, error) {
file, err := f.fs.Open(f.metadataPathForVolumeID(volumeID))
file, err := os.Open(f.metadataPathForVolumeID(volumeID))
if err != nil {
// don't leak through error types from fs.Open - wrap with ErrNotFound
// if calling Open fails, as this indicates an invalid path
Expand Down Expand Up @@ -158,6 +177,11 @@ func (f *Filesystem) ReadMetadata(volumeID string) (metadata.Metadata, error) {
}

func (f *Filesystem) WriteMetadata(volumeID string, meta metadata.Metadata) error {
// Ensure the volume directory exists.
if err := f.ensureVolumeDirectory(volumeID); err != nil {
return err
}

metaBytes, err := json.Marshal(meta)
if err != nil {
// if it's an error type we don't recognise, wrap it with %v to prevent
Expand All @@ -171,11 +195,6 @@ func (f *Filesystem) WriteMetadata(volumeID string, meta metadata.Metadata) erro
func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) {
existingMeta, err := f.ReadMetadata(meta.VolumeID)
if errors.Is(err, ErrNotFound) {
// Ensure directory structure for the volume exists
if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now called in WriteMetadata instead.

return false, err
}

if err := f.WriteMetadata(meta.VolumeID, meta); err != nil {
return false, err
}
Expand All @@ -185,13 +204,6 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) {

// If the volume context has changed, should write updated metadata
if !apiequality.Semantic.DeepEqual(existingMeta.VolumeContext, meta.VolumeContext) {
// Ensure directory structure for the volume exists - this will probably do
// nothing, but it helps avoid any weird edge cases we could find ourselves in &
// is an inexpensive operation.
if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil {
return false, err
}

f.log.WithValues("volume_id", meta.VolumeID).Info("volume context changed, updating file system metadata")
existingMeta.VolumeContext = meta.VolumeContext
if err := f.WriteMetadata(existingMeta.VolumeID, existingMeta); err != nil {
Expand All @@ -207,29 +219,14 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) {
// ensureVolumeDirectory ensures the directory structure for the volume exists.
// If the directories already exist, it will do nothing.
func (f *Filesystem) ensureVolumeDirectory(volumeID string) error {
if err := os.MkdirAll(f.volumePath(volumeID), 0644); err != nil {
return err
}

// Data directory should be read and execute only to the fs user and group.
if err := os.MkdirAll(f.dataPathForVolumeID(volumeID), 0550); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved to WriteFiles, since we don't need it in WriteMetadata (the other caller of this function).

return err
}

return nil
return os.MkdirAll(f.volumePath(volumeID), volumeFolderMode)
}

// WriteFiles writes the given data to filesystem files within the volume's
// data directory. Filesystem supports changing ownership of the data directory
// to a custom gid.
func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) error {
// Ensure the full directory structure for the volume exists.
// This already happens in RegisterMetadata, however, when a driver starts up and reads
// the metadata files from the existing tmpfs to re-populate the manager, RegisterMetadata
// is not called again (it is only invoked by driver/nodeserver.go when a pod is first processed
// during NodePublishVolume).
// There is a very slim chance we could end out in a weird situation where the metadata
// file exists but the data directory does not, so re-run ensureVolumeDirectory just to be safe.
// Ensure the volume directory exists.
if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil {
return err
}
Expand All @@ -239,9 +236,14 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte)
return err
}

// Data directory should be read and execute only to the fs user and group.
if err := os.MkdirAll(f.dataPathForVolumeID(meta.VolumeID), volumeDataFolderMode); err != nil {
return err
}

// If a fsGroup is defined, Chown the directory to that group.
if fsGroup != nil {
if err := os.Chown(f.dataPathForVolumeID(meta.VolumeID), -1, int(*fsGroup)); err != nil {
if err := os.Lchown(f.dataPathForVolumeID(meta.VolumeID), -1, int(*fsGroup)); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chown vs Lchown:

If the file is a symbolic link, it changes the uid and gid of the link itself.

This seems more secure. However, since the dataPathForVolumeID is a folder, this should not change anything.

return fmt.Errorf("failed to chown data dir to gid %v: %w", *fsGroup, err)
}
}
Expand All @@ -260,7 +262,7 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte)
// If a fsGroup is defined, Chown all files in the timestamped directory.
for filename := range files {
// Set the uid to -1 which means don't change ownership in Go.
if err := os.Chown(filepath.Join(f.dataPathForVolumeID(meta.VolumeID), tsDirName, filename), -1, int(*fsGroup)); err != nil {
if err := os.Lchown(filepath.Join(f.dataPathForVolumeID(meta.VolumeID), tsDirName, filename), -1, int(*fsGroup)); err != nil {
return err
}
}
Expand All @@ -276,7 +278,7 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte)

// ReadFile reads the named file within the volume's data directory.
func (f *Filesystem) ReadFile(volumeID, name string) ([]byte, error) {
file, err := f.fs.Open(filepath.Join(f.dataPathForVolumeID(volumeID), name))
file, err := os.Open(filepath.Join(f.dataPathForVolumeID(volumeID), name))
if err != nil {
// don't leak through error types from fs.Open - wrap with ErrNotFound
// if calling Open fails, as this indicates an invalid path
Expand All @@ -300,11 +302,7 @@ func (f *Filesystem) dataPathForVolumeID(id string) string {
}

func (f *Filesystem) volumePath(id string) string {
return filepath.Join(f.tempfsPath(), id)
}

func (f *Filesystem) tempfsPath() string {
return filepath.Join(f.baseDir, "inmemfs")
return filepath.Join(f.basePath, id)
}

func makePayload(in map[string][]byte) map[string]util.FileProjection {
Expand Down
Loading