-
Notifications
You must be signed in to change notification settings - Fork 236
modifications to extract filesystem for kbuild #191
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments.
pkg/util/cloud_prepper.go
Outdated
} | ||
|
||
return path, getFileSystemFromReference(ref, p.ImageSource, path) | ||
return path, GetFileSystemFromReference(ref, p.ImageSource, path, []string{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to put nil here.
pkg/util/docker_utils.go
Outdated
|
||
for _, layer := range layers { | ||
if err = UnTar(bytes.NewReader(layerMap[layer]), target); err != nil { | ||
if err = UnTar(bytes.NewReader(layerMap[layer]), target, []string{}); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here.
return err | ||
} | ||
} else { | ||
// In some cases, MkdirAll doesn't change the permissions, so run Chmod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually umask related, but I suppose if the dirs already exist it wouldn't change the permissions either
}) | ||
} | ||
|
||
func checkWhitelist(target string, whitelist []string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want something more precise than prefix matching. Something like /foo would block /foobar when it should only block /foo/bar
func HasFilepathPrefix(path, prefix string) bool { | ||
path = filepath.Clean(path) | ||
prefix = filepath.Clean(prefix) | ||
pathArray := strings.Split(path, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use filepath.Split here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it only splits along the last separator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Right.
|
||
target := filepath.Join(path, header.Name) | ||
// Make sure the target isn't part of the whitelist | ||
if checkWhitelist(target, whitelist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A debug log here will come in handy later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it in the checkWhitelist function
pkg/util/tar_utils.go
Outdated
|
||
func walkAndRemove(p string) error { | ||
return filepath.Walk(p, func(path string, info os.FileInfo, err error) error { | ||
if e := os.Chmod(path, 0777); e != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to chmod before removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I was trying something out and meant to remove that
Extract symlinks from the tarball, and also add support for a whitelist (files that would be ignored during extraction)