Refactor filesystem.go and adapt tests to use a real file system #71
Conversation
cert-manager-prow
bot
added
dco-signoff: yes
| // 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") |
There was a problem hiding this comment.
Starting from this PR, we no longer support the weird relative path notation. All paths have to be absolute.
There was a problem hiding this comment.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Tim Ramlot <[email protected]>
inteon
force-pushed
the
filesystem_tests
branch
3 times, most recently
from
ca97148 to
e846f8c
Compare
| // 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 { |
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
This is now called in WriteMetadata instead.
| } | ||
|
|
||
| // Data directory should be read and execute only to the fs user and group. | ||
| if err := os.MkdirAll(f.dataPathForVolumeID(volumeID), 0550); err != nil { |
There was a problem hiding this comment.
This was moved to WriteFiles, since we don't need it in WriteMetadata (the other caller of this function).
Adds tests to prevent bugs like #70 in the future.
Refactors filesystem.go to only accept absolute paths instead of the weird current relative absolute paths (due to
os.DirFS("/")), fixing #15.NOTE: we will have to modify all csi-drivers that use this lib to only accept/ input absolute paths