Add NormalizeUnpackFileModes option to Packer struct
#53
Conversation
The inspiration for this change stems from the way git handles files and normalizes permissions. This change marks the first step to allow go-slug to similarly resolve file permissions when unpacking tarred contents. In summary, we normalize all regular files to allow the owner to read/write the file, while also preserving permissions for executables and symlinks. This normalization process also disallows certain files like named pipes, devices, sockets, etc.
| return i.Typeflag == tar.TypeSymlink | ||
| } | ||
|
|
||
| // IsDirectory describes whether the file being unpacked is a directory | ||
| func (i UnpackInfo) IsDirectory() bool { | ||
| func (i *UnpackInfo) IsDirectory() bool { |
There was a problem hiding this comment.
Can you say more about why you're changing these immutable bool-returning IsSomething() methods to a pointer receiver? I thought Go auto-dereferenced, so you can just directly call value methods if you're holding a pointer.
There was a problem hiding this comment.
Normally I've followed the styling that disallows mixing receivers (see Go's guidance). So given I needed a pointer receiver to configure an instance of UnpackInfo, I changed all of the methods for consistency.
There was a problem hiding this comment.
ah, okay! Solid answer, thank you.
internal/unpackinfo/filemode.go
Outdated
| // Regular represent non-executable files. Please note this is not | ||
| // the same as golang regular files, which include executable files. |
There was a problem hiding this comment.
Hmm. Do you think it's worth using a slightly different name than Go does to avoid confusion? If so, I suggest "Plain".
internal/unpackinfo/filemode.go
Outdated
| case Regular: | ||
| return fs.FileMode(0644), nil | ||
| case Dir: | ||
| return fs.ModePerm | fs.ModeDir, nil |
There was a problem hiding this comment.
This made me doubt myself and I ran in circles about it for a while, but! I think we actually want directories to be 755 (world read+searchable), not 777 (world read+search+writeable). Thoughts?
There was a problem hiding this comment.
In truth I followed the permissions set by go-git. I wonder if there is a specific use case in git that requires dirs with r+w+x for all groups/users. I think for the purposes of Terraform configuration this would be irrelevant so we can get by with 0755. I will double check this to be sure.
There was a problem hiding this comment.
🤔 I wonder if there's a mask being applied somewhere later down the chain. Maybe do a real-world test and see what comes out; this is a case where I'm dubious about trusting what the code claims it's doing.
sebasslash
force-pushed
the
sebasslash/ipl-5467_normalize-perms-unpack
branch
2 times, most recently
from
b00a600 to
854fa1c
Compare
sebasslash
force-pushed
the
sebasslash/ipl-5467_normalize-perms-unpack
branch
from
854fa1c to
cefa5f0
Compare
|
I don't feel this needs to be a new option. Why wouldn't we make it the default behavior? |
A recent change was introduced that preserves a file's original atime, mtime and mode when unpacked from a tarball. This consequently shifted responsibility to the slug's consumer to ensure each untarred file has the expected permissions. Inevitably this can lead to unpredictable errors, particularly if slugs are created by unknown sources and are expected to be handled uniformly.
This PR aims to add a new packer option,
NormalizeUnpackFileModes, that configures go-slug to normalize file modes when unpacking configuration files. This change aims to behave closely to how git normalizes permissions. The permissions for each type of file are encoded as follows:All other file types are unsupported and result in an error: named pipes, char devices, sockets, etc.
Example Usage: