Remove block preventing us from making hardlinks to symlinks #1187
Conversation
katiewasnothere
force-pushed
the
allow_hardlinks_to_symlinks
branch
from
12a1222 to
27ad71d
Compare
|
Did we verify any of the troublesome images that were erroring here now work (and nothing wonky in the guest for the link)? |
I didn't check with any specific container images. I did check with test tar files and the test included in the PR validate the PR. With the test tar files I made I validated that there was not break out. |
|
Sure, just wanted to see if we've tried a container image that failed with the old behavior to be safe |
dcantah
left a comment
dcantah
left a comment
There was a problem hiding this comment.
I still think we should put an image that has a hardlink -> symlink in one of its layers up on our cplatpublic registry and make a cri-containerd test to be safe. Changes here lgtm though.
We discussed this and agreed that a test like this would be best suited for a different PR where we can include other extensive tests of the tar functionality. |
| switch oldfile.Mode & format.TypeMask { | ||
| case format.S_IFDIR, format.S_IFLNK: | ||
| return fmt.Errorf("%s: link target cannot be a directory or symlink: %s", newname, oldname) | ||
| case format.S_IFDIR: |
There was a problem hiding this comment.
So with this change we are setting our policy as "hard links to symlinks will point to symlink itself than the file which symlink points to". Do we know if containerd upstream also follows the same policy?
There was a problem hiding this comment.
Yes, that's correct. The same is done in containerd, see https://github.com/containerd/containerd/blob/10d9d1a60fe797b51599cea9f0881a256a98b469/archive/tar.go#L736
There was a problem hiding this comment.
Awesome. Thanks!
Can we add a comment here saying that we follow containerd upstream policy and provide this link? It would help in the future.
katiewasnothere
force-pushed
the
allow_hardlinks_to_symlinks
branch
from
27ad71d to
5f997e2
Compare
Signed-off-by: Kathryn Baldauf <[email protected]>
katiewasnothere
force-pushed
the
allow_hardlinks_to_symlinks
branch
from
5f997e2 to
510109e
Compare
Related work items: microsoft#1067, microsoft#1097, microsoft#1119, microsoft#1170, microsoft#1176, microsoft#1180, microsoft#1181, microsoft#1182, microsoft#1183, microsoft#1184, microsoft#1185, microsoft#1186, microsoft#1187, microsoft#1188, microsoft#1189, microsoft#1191, microsoft#1193, microsoft#1194, microsoft#1195, microsoft#1196, microsoft#1197, microsoft#1200, microsoft#1201, microsoft#1202, microsoft#1203, microsoft#1204, microsoft#1205, microsoft#1206, microsoft#1207, microsoft#1209, microsoft#1210, microsoft#1211, microsoft#1218, microsoft#1219, microsoft#1220, microsoft#1223
3 participants
Addresses #1120. This PR removes the previous block preventing hardlinks from being made to symlinks. Since symlinks are evaluated dynamically in the container based on the current root and tar2ext4 bounds hard link targets within the root of the inode structure, we don't need additional checking of any symlink targets in this scenario.
Additionally adds test to verify behavior.
Signed-off-by: Kathryn Baldauf [email protected]