GoogleContainerTools / kaniko

Build Container Images In Kubernetes
Apache License 2.0
14.89k stars 1.44k forks source link

Added --chmod for ADD and COPY commands. Fixes #2850 and #1751 #3119

Closed mschneider82 closed 7 months ago

mschneider82 commented 7 months ago

Fixes #2850 and #1751

Description

I have added --chmod feature for ADD and COPY command.

INFO[0004] ADD --chmod=644 https://xxx/test.gpg /etc/apt/trusted.gpg.d/ 
INFO[0004] RUN ls -la /etc/apt/trusted.gpg.d/test.gpg 
INFO[0007] Cmd: /bin/sh                                 
INFO[0007] Args: [-c ls -la /etc/apt/trusted.gpg.d/test.gpg] 
INFO[0007] Running: [/bin/sh -c ls -la /etc/apt/trusted.gpg.d/test.gpg] 
-rw-r--r-- 1 root root 1545 May  5  2023 /etc/apt/trusted.gpg.d/test.gpg

Submitter Checklist

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

Reviewer Notes

Release Notes

- kaniko now supports setting --chmod on ADD and COPY commands.
google-cla[bot] commented 7 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

mschneider82 commented 7 months ago

@aaron-prindle can you please review?

aaron-prindle commented 7 months ago

@mschneider82 thanks for the PR here! The PR itself LGTM, currently there is some test failures but I believe they are flakes and unrelated. Can you rebase the PR and resubmit to see if we can get the CI/CD tests green so we can merge this? Thanks

mschneider82 commented 7 months ago

Still fails but i have tested the new Integration with LOCAL env set.

Error below from Integration Test on GitHub, with this stat command i check the permissions of the Default ADD command without --chmod

It uses the src as permissions source for chmod. It was 775 on my git Checkout, maybe different in github

            INFO[0000] Running: [/bin/sh -c test "$(stat -c "%a" /path/file666)" = "666"] 
            INFO[0000] Taking snapshot of full filesystem...        
            INFO[0000] No files were changed, appending empty layer to config. No layer added to image. 
            INFO[0000] RUN test "$(stat -c "%a" /path/file)" = "775" 
            INFO[0000] Cmd: /bin/sh                                 
            INFO[0000] Args: [-c test "$(stat -c "%a" /path/file)" = "775"] 
            INFO[0000] Running: [/bin/sh -c test "$(stat -c "%a" /path/file)" = "775"] 
            error building image: error building stage: failed to execute command: waiting for process to exit: exit status 1
FAIL
FAIL    github.com/GoogleContainerTools/kaniko/integration  488.942s
FAIL
mschneider82 commented 7 months ago

I have removed the mentioned checks for default behaiviour for the ADD and COPY without a --chmod command. the integration test now only checks the --chmod feature.

@aaron-prindle please trigger https://github.com/GoogleContainerTools/kaniko/actions/runs/8758320746

mschneider82 commented 7 months ago

I dont see whats wrong with the Integration test

aaron-prindle commented 7 months ago

I believe this is a test flake in our integration tests @mschneider82. In the past 2-3 days I have seen the below error on another PR (which was only bumping deps) as well:

        cmd.go:40: [container-diff diff --no-cache daemon://localhost:5000/docker-dockerfile_test_cache_copy_oci daemon://localhost:5000/kaniko-dockerfile_test_cache_copy_oci -q --type=file --type=metadata --json]
        cmd.go:41: time="2024-04-15T03:01:15Z" level=error msg="error retrieving image daemon://localhost:5000/docker-dockerfile_test_cache_copy_oci: retrieving image from daemon: Error response from daemon: reference does not exist"

I am still trying to root cause this atm. If I can identify the issue soon I will submit a fix and rebase (if this is related to our test infra) and if not I can merge this as is (with test flake) as I don't believe the changes in this PR are related to the test failures

flake tracking issue: https://github.com/GoogleContainerTools/kaniko/issues/3124

previous runs with this flake (the same PR would then pass later): https://github.com/GoogleContainerTools/kaniko/actions/runs/8682504136/job/23807123764#step:7:73

mschneider82 commented 7 months ago

Ok feel free to merge then, i use this patched Version since days without an issue

aaron-prindle commented 7 months ago

Just merged, #3131. If you rebase and push we can hopefully see some additional error information

mschneider82 commented 7 months ago

Ok it fails because the docker command does not support chmod without buildkit:

            the --chmod option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled

in .github/workflows/integration-tests.yaml its disabled DOCKER_BUILDKIT: '0' maybe try to switch it on?

mschneider82 commented 7 months ago

ok doing buildkit=1 for integration is a longer task to fix, but this should be a seperated task. The lagacy build will be deprecated, to it should compare to buildkit instead of legacy docker build.

mschneider82 commented 7 months ago

I found the chmod container build, with docker it had 11 layers with kaniko just 5?

https://github.com/GoogleContainerTools/kaniko/actions/runs/8780308512/job/24089955636?pr=3119#step:7:765

To debug this it would be helpful if the build from kaniko would be also printed. currently in images.go the output is just printed when the image build fails, but not returned, in this case it would be helpful to return it and also print it when the layercheck has a difference detected.

mschneider82 commented 7 months ago

@aaron-prindle OK i got it green, i had to enable DOCKER_BUILDKIT=1 for just this single integration test via envsMap in images.go also added the layers difference of 6. I have rebased everything. In future i think many more tests need to be fixed to use buildkit instead of classic build.