fix: kaniko dir env unused

[jdockerty]

Description

Relating to the comment made in #1997.

Previously, I made an oversight on what to be passed into the checkKanikoDir func, it was only the configuration option. Instead, it should have been a choice between the environment variable and the option. I have decided that the flag will take precedence, but this can easily be altered depending on any further discussion.

There was also an underlying problem of seeing file already exists and invalid cross-device link errors after the above was fixed, this seems to have been addressed by utilising the CopyDir func as part of the utils package. This enables the moving of the KanikoDir contents, regardless of whether it is destined for another partition or not.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes unit tests
• Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

• The code flow looks good.
• Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Fixes the kaniko-dir flag, this now takes precedence over the KANIKO_DIR environment variable.

[imjasonh]

Otherwise LGTM! Is there a test that's reasonably easy to add for this? It's okay if not.

[jdockerty]

Will investigate the failing test, I believe the error on passing the kaniko-dir flag resulting in exit status 1 Error: rename /kaniko /not-kaniko: file exists is something other people were running into.

I'll dig into this a bit further when I have some spare time.

[jdockerty]

I've updated the description to reflect another underlying issue which surfaced after the change made in c04538d, which should be fixed from this PR, with some extra explanation.

[jdockerty]

Is there a test that's reasonably easy to add for this? It's okay if not.

@imjasonh There's nothing that really springs to mind for a test in this case, the previous one that we created from the other PR passes and the checkKanikoDir is now using another function from the util package, which is already heavily tested.

Is there any sort of edge case which you can think of which we should cover? I may well be missing something here, but can't think of anything 😄

[tspearconquest]

This seems not to work with non-root containers. Copying the specified directory to the default location won't work in this case, so we need a more comprehensive solution for the issue.

It appears to my untrained eye that the problem is here because it uses the environment variable or the constant value, instead of the environment variable or the config value, and so on lines 37, 41, and 45, the /kaniko path is still being used despite me giving a different path with the --kaniko-dir flag.

[tspearconquest]

I was able to partially work around the issue by specifying the alternate directory in both the environment variable and the command line flag, and by using a dev release of the image, however I now get Error: unlinkat //kaniko: permission denied

I assume this is due to the os.RemoveAll call at

kaniko/cmd/executor/cmd/root.go

Line 259 in 323e616

if err := os.RemoveAll(constants.DefaultKanikoPath); err != nil {

which won't work with non-root builds:

$ docker run --rm -it --entrypoint "" registry.gitlab.com/armed-atk/development/devsecops/images/kaniko/executor:test /bin/sh
/workspace $ ls -ld /kaniko
drwxr-xr-x    2 kaniko   kaniko        4096 Aug  2 06:39 /kaniko
/workspace $ id
uid=65532(kaniko) gid=65532(kaniko)
/workspace $ rm -rf /kaniko
rm: can't remove '/kaniko': Permission denied
/workspace $

[tspearconquest]

Good morning, today I updated my dockerfile to run as root, and I was able to build the image without issue using Kaniko to build it because the /kaniko path is able to be deleted properly by the above code.

I ran into other issues with the built image which appear to be unrelated to this pull request, so I'll open a new issue for that.