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.

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

Cannot COPY file to existing directory without trailing slash #365

Closed
Zetten opened this issue Sep 26, 2018 · 2 comments
Closed

Cannot COPY file to existing directory without trailing slash #365

Zetten opened this issue Sep 26, 2018 · 2 comments
Labels
kind/bug Something isn't working

Comments

@Zetten
Copy link
Contributor

Zetten commented Sep 26, 2018

Actual behavior
A Dockerfile with content

RUN mkdir -p /path/to/foo
COPY sourcefile /path/to/foo

will fail to build in Kaniko, with the error:
error building image: error building stage: open /path/to/foo: is a directory

Expected behavior
docker build is rather more lenient with the target for COPY commands, and will create the file in the destination directory: /path/to/foo/sourcefile.

Additional Information
I've tried tracking down the problem through the DestinationFilepath function in command_util.go. This checks whether the destination is a directory using the IsDestDir function. However IsDestDir is quite naive and string-based, and only checks for the existence of a trailing slash or cwd marker - which are not present in the example above.

Changing IsDestDir seems like it may have side effects in other places, but I managed to test successfully with a new function, replacing the IsDestDir call in DestinationFilepath:

func IsDirectory(path string) bool {
	fileInfo, err := os.Stat(path)
	if err != nil{
		return false
	}
	return fileInfo.IsDir()
}

It's not clear to me whether this is the best approach, nor how to properly test it. I'm happy to create a PR with some feedback!

@priyawadhwa priyawadhwa added the kind/bug Something isn't working label Sep 27, 2018
@priyawadhwa
Copy link
Collaborator

@Zetten I agree that currently IsDestDir is naive and should be improved. I think the string parsing is necessary in the case where we have something like

COPY file /maybe/a/dir

and /maybe/a/dir doesn't exist yet, so the trailing slash tells us if it's meant to be a directory or a file.

Perhaps we should check if the directory exists first, and if not then check the string? If you're still interested in opening a PR for this, that would be great!

Zetten added a commit to Zetten/kaniko that referenced this issue Oct 11, 2018
Add a check for FileInfo to determine whether a given string is a
directory path. If any error occurs, fall back to the naive string
check.

Fixes GoogleContainerTools#365
@Zetten
Copy link
Contributor Author

Zetten commented Oct 11, 2018

The PR tries to avoid any repercussions of changing IsDestDir, but it's used in a few places and not always to check real filesystems. I'm not so hot with go but I've tried to keep it as simple and defensive as I can. For the same reason, I've also missed out testing this - it hasn't broken the existing stuff, but with some guidance I can try adding a new test.

Zetten added a commit to Zetten/kaniko that referenced this issue Oct 11, 2018
Add a check for FileInfo to determine whether a given string is a
directory path. If any error occurs, fall back to the naive string
check.

Fixes GoogleContainerTools#365
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants