Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix flake TestRun/Dockerfile_test_copy_symlink #1030

Merged
merged 2 commits into from
Feb 6, 2020
Merged

fix flake TestRun/Dockerfile_test_copy_symlink #1030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cd68d2
fix flake in copy symlink
tejal29 Feb 5, 2020
9e17ffd
fix unit tests
tejal29 Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion integration/dockerfiles/Dockerfile_test_copy_symlink
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,13 @@ FROM busybox as t
RUN echo "hello" > /tmp/target
RUN ln -s /tmp/target /tmp/link

## Relative link
RUN cd tmp && ln -s target relative_link

## Relative link with paths
RUN mkdir workdir && cd workdir && ln -s ../tmp/target relative_dir_link

FROM scratch
COPY --from=t /tmp/link /tmp
COPY --from=t /tmp/link /tmp/
COPY --from=t /tmp/relative_link /tmp/
COPY --from=t /workdir/relative_dir_link /workdir/
4 changes: 2 additions & 2 deletions pkg/commands/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
}
c.snapshotFiles = append(c.snapshotFiles, copiedFiles...)
} else if util.IsSymlink(fi) {
// If file is a symlink, we want to create the same relative symlink
exclude, err := util.CopySymlink(fullPath, destPath, c.buildcontext)
// If file is a symlink, we want to copy the target file to destPath
exclude, err := util.CopySymlink(fullPath, destPath, c.buildcontext, uid, gid)
if err != nil {
return err
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/executor/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,9 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) {
}
for _, p := range filesToSave {
logrus.Infof("Saving file %s for later use", p)
util.CopyFileOrSymlink(p, dstDir)
if err := util.CopyFileOrSymlink(p, dstDir); err != nil {
return nil, err
}
}

// Delete the filesystem
Expand Down
16 changes: 10 additions & 6 deletions pkg/util/fs_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ func CopyDir(src, dest, buildcontext string, uid, gid int64) ([]string, error) {
}
} else if IsSymlink(fi) {
// If file is a symlink, we want to create the same relative symlink
if _, err := CopySymlink(fullPath, destPath, buildcontext); err != nil {
if _, err := CopySymlink(fullPath, destPath, buildcontext, uid, gid); err != nil {
return nil, err
}
} else {
Expand All @@ -599,12 +599,12 @@ func CopyDir(src, dest, buildcontext string, uid, gid int64) ([]string, error) {
}

// CopySymlink copies the symlink at src to dest
func CopySymlink(src, dest, buildcontext string) (bool, error) {
func CopySymlink(src, dest, buildcontext string, uid int64, gid int64) (bool, error) {
if ExcludeFile(src, buildcontext) {
logrus.Debugf("%s found in .dockerignore, ignoring", src)
return true, nil
}
link, err := os.Readlink(src)
link, err := filepath.EvalSymlinks(src)
if err != nil {
return false, err
}
Expand All @@ -616,7 +616,7 @@ func CopySymlink(src, dest, buildcontext string) (bool, error) {
if err := createParentDirectory(dest); err != nil {
return false, err
}
return false, os.Symlink(link, dest)
return CopyFile(link, dest, buildcontext, uid, gid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change from link to copy? I'm struggling to connect this change to a flaky test.

Copy link
Contributor Author

@tejal29 tejal29 Feb 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was flaky because of 2 reasons

  1. The link value was /tmp/target.
    We were linking kaniko/0/tmp/link to /tmp/target - should have been /kaniko/0/tmp/link
  2. The target file /tmp/target was getting wiped out before call to os.Symlink(link, dest) . - this caused flakiness.
    (I still don't know why this is caused. However, imitating docker by actually creating the destlink as a regular file with contents of target file fixed the flakiness)

In the description, i mentioned i fixed the CopySymlink to replicate docker behavior. Docker copies the target file to destination link and hence i replaced os.Symlink to CopyFile.

}

// CopyFile copies the file at src to dest
Expand Down Expand Up @@ -789,11 +789,15 @@ func getSymlink(path string) error {
func CopyFileOrSymlink(src string, destDir string) error {
destFile := filepath.Join(destDir, src)
if fi, _ := os.Lstat(src); IsSymlink(fi) {
link, err := os.Readlink(src)
link, err := EvalSymLink(src)
if err != nil {
return err
}
return os.Symlink(link, destFile)
linkPath := filepath.Join(destDir, link)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this code a bit? I'm having trouble wraping my head around it.

It looks like linkPath is the resolved path of src joined with destDir. I'm confused how we can be sure this path exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was error 1 mentioned in here

This ensures, we are copying/creating the required file in kaniko sratch workspace.

if err := createParentDirectory(destFile); err != nil {
return err
}
return os.Symlink(linkPath, destFile)
}
return otiai10Cpy.Copy(src, destFile)
}
Expand Down
8 changes: 2 additions & 6 deletions pkg/util/fs_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,16 +835,12 @@ func TestCopySymlink(t *testing.T) {
if err := os.Symlink(tc.linkTarget, link); err != nil {
t.Fatal(err)
}
if _, err := CopySymlink(link, dest, ""); err != nil {
if _, err := CopySymlink(link, dest, "", DoNotChangeUID, DoNotChangeGID); err != nil {
t.Fatal(err)
}
got, err := os.Readlink(dest)
if err != nil {
if _, err := os.Lstat(dest); err != nil {
t.Fatalf("error reading link %s: %s", link, err)
}
if got != tc.linkTarget {
t.Errorf("link target does not match: %s != %s", got, tc.linkTarget)
}
})
}
}
Expand Down