Refactor filesystem.go and adapt tests to use a real file system#71
Refactor filesystem.go and adapt tests to use a real file system#71inteon wants to merge 1 commit intocert-manager:mainfrom
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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
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 should be more secure. However, since the dataPathForVolumeID is a folder, this should not change anything.
| 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).
1 participant
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