Respect zipped symlinks #1140
Respect zipped symlinks #1140
Conversation
chore: Release v0.8.0
chore: v0.8.1 Release
chore: Release v0.9.0
chore: Release v0.10.0
chore: Release v0.11.0
Releasing v0.12.0
SAM CLI v0.13 release
Release: Version 0.14.0
chore: Version bump SAM CLI to 0.14.1 (#1082)
Release: v0.14.2
v0.15.0 Release
…sam-cli into respect-zipped-symlinks
|
@sanathkr Rebased and ready. Thanks for reviewing it! |
There was a problem hiding this comment.
@bubba-h57 This looks good over all. Just two small comments.
Additional question:
- Did you do any testing to validate this does solve symlinks in zip are not retained on the docker image created when using Layers #878?
| A response regarding whether the ZipInfo defines a symlink or not. | ||
| """ | ||
|
|
||
| return (file_info.external_attr >> 28) == 0xA |
There was a problem hiding this comment.
Should this be using the S_IFLNK constant from above?
Why not use stat. S_IFLNK here instead?
There was a problem hiding this comment.
This is not looking at a file on the filesystem. It is looking at a number of bytes in the file_info entry, the External Attributes for the file. See The zip format's external file attribute If you want to dig into it. But the short answer is because it isn't a file.
There was a problem hiding this comment.
I see. You made a constant S_IFLNK = 0xA above. Can you either remove it the constant or replace the 0xA here with S_IFLNK. Otherwise, it looks like an unused constant.
There was a problem hiding this comment.
^ that was with regard to pythons stat.S_ISLNK() ... I have no good excuse for not comparing to the S_IFLNK = 0xA constant defined directly above the function. :-)
|
|
||
| # If the link already exists, delete it or symlink() fails | ||
| if os.path.lexists(link_name): | ||
| os.remove(link_name) |
There was a problem hiding this comment.
Will this have any (negative) side effects? I can't think of any at the moment but asking the question to spark a discussion.
There was a problem hiding this comment.
I tested it fairly extensively and can not think of any negative side effects that might pop up. The unzip intends to overwrite anything that is there regardless ... we will simply delete it prior to writing.
|
We did indeed test it to solve #878 ... that issue was the whole point of interest for us. :-) |
WOOT! I just wanted to make sure. If you didn't, I was going to pull down and verify (mainly why I was asking). |
|
Well, I'm all for you testing it also. It has been a while since I looked it. |
|
I ended up needing to revert this. Details can be found here. We will need to revisit the implementation and either way for the Py2.7 to be depreciated or update this to not work on Py2.7 in windows. |
This reverts commit 360d90a. This commit originally reverted aws#1140 due to os.symlink not being available on windows in Python2.7 stdlib, details here: aws#1315 (comment). We recently removed Python2.7 support in aws#1416, so this commit is a revert of the revert, with some additional black formating to make `make pr` pass.
Issue #878 :
Description of challenge:
SAM CLI currently used the
Lib/zipfile.pyfrom the Python Standard Library. Unfortunately, that library has not been kept up to date with the .ZIP File Format Specification and while it does handle basic archival CRUD, there are a number of features that have become standard and which it does not support.The greatest interest to us is that of properly handling symbolic links. It can neither archive them properly (see #477) nor can it extract them properly. This is particularly important now that we are compiling libraries for custom layers. Given the limited file space available to Lambda Functions, having
/libs/*.sofiles all copied instead of symlinked exponentially increases the uncompressed layer size.Lambda itself handles the archived symlinks properly; however, attempting to test the same layers with
sam localfails. This forces fat layers if the developer desires to use 'sam local'Description of changes:
The
unzip()function insamcli/local/lambdafn/zip.pyis modified to identify and properly handle any symlinks. The StandardzipfileLibrary continues to do all of the heavy work with a wrapper around it to handle the identification and extraction of the zipfiles. Regular files and directories continue to be extracted in precisely the same manner as before. All changes are documented with appropriate docblocks.tests/unit/local/lambdafn/test_zip.pyhad to be modified in order to insert a symlink into the test archive as well as testing that the symlink was properly extracted. I did not see anyintegrationnorfunctionaltests that needed modification.Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.