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 |
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. |
| @@ -620,8 +620,8 @@ func (w *Writer) Link(oldname, newname string) error { | |||
| return err | |||
| } | |||
| 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 <kabaldau@microsoft.com>
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
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 kabaldau@microsoft.com