Merge slice fixes into main #340
Conversation
Signed-off-by: Emily Casey <ecasey@vmware.com>
* Add matched symlinks to the slice instead of the resolved target * Slice layers should not include files globbing through a symlink * Only clean up links to dirs, not the target dirs * Normalize uid/gid of the parents of a matched file only if they are within the app dir Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
* Removes unused test helpers Signed-off-by: Emily Casey <ecasey@vmware.com>
…complete Signed-off-by: Emily Casey <ecasey@vmware.com>
Fix handling of symlinks in slices
* Fixes cache log messages Signed-off-by: Emily Casey <ecasey@vmware.com>
Fixes tar reuse logic
refactors windows logic to use new layers package Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
ekcasey
force-pushed
the
merge-slice-fix
branch
10 times, most recently
from
a8e0fbe to
963d735
Compare
Signed-off-by: Emily Casey <ecasey@vmware.com>
Docker copy removes file attributes for windows symlink directories that need to be preserved for certain tests Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
| func createSymlink(hdr *tar.Header) error { | ||
| var flags uint32 = symbolicLinkFlagAllowUnprivilegedCreate | ||
| if attrStr, ok := hdr.PAXRecords[hdrFileAttributes]; ok { | ||
| attr, err := strconv.ParseUint(attrStr, 10, 32) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to parse file attributes for header %q", hdr.Name) | ||
| } | ||
| if attr&syscall.FILE_ATTRIBUTE_DIRECTORY != 0 { | ||
| flags |= syscall.SYMBOLIC_LINK_FLAG_DIRECTORY | ||
| } | ||
| } | ||
|
|
||
| name, err := syscall.UTF16PtrFromString(hdr.Name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| target, err := syscall.UTF16PtrFromString(hdr.Linkname) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return syscall.CreateSymbolicLink(name, target, flags) | ||
| } |
There was a problem hiding this comment.
It's a bummer to have to do all this to work around the divergent Windows/POSIX golang behavior. After reading that thread it sounds like a wontfix so os.Symlink won't change anytime soon. This implementation feels like it follows one of the suggestions there:
For the few programs that actually do need to create symlinks to nonexistent targets, windows.CreateSymbolicLink seems sufficient, and it's easy enough for users to write their own portable wrappers using +build constraints.
Workaround broken directory symlink behavior in docker build Signed-off-by: Micah Young <ymicah@vmware.com>
Fix make docker-run-windows branch merge-slice-fix
Signed-off-by: Emily Casey <ecasey@vmware.com>
There was a problem hiding this comment.
@ekcasey this looks great! I'm not too familiar with how slices worked before, but the new organization is very clear and easy to follow.
| it("creates a layer from the directory", func() { | ||
| // parent layers should have uid/gid matching the filesystem | ||
| // the dir and it's children should have normalized uid/gid | ||
| assertTarEntries(t, dirLayer.TarPath, append(parents(t, dir), []*tar.Header{ |
There was a problem hiding this comment.
I might be missing something but it doesn't seem that we test the permissions of the parent directory anywhere, no? Is this possible?
There was a problem hiding this comment.
The expected parent headers have UID/GID set on them but the assertions are missing (!) and we don't do this for windows headers. I will clean this up.
https://github.com/buildpacks/lifecycle/blob/merge-slice-fix/layers/layers_unix_test.go#L18-L20
There was a problem hiding this comment.
Actually, we do test UID/GID but only for linux layers. https://github.com/buildpacks/lifecycle/blob/merge-slice-fix/layers/layers_unix_test.go#L28-L36
We currently aren't setting the SID on windows layer entries properly (looks like setting header.Uid doesn't do the trick) but that was true before this PR and I would like to fix it separately. I believe the SID should be set to the value of CNB_USER_ID for windows layers using PAX records.
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
This PR includes slices fixes for symlinks from 0.8.1 (#335). Additional work was required to fix symlink handling in windows. The archive package now adds PAX records to windows archives headers so that we can properly make directory or file type symlinks even when the link target does not exist (for context: golang/go#39183).
This PR runs the windows unit and acceptance tests in GHA without the
docker-run-windowswrapper. That wrapper removes permissions from test fixtures that are required to properly test handling of directory symlinks.