Add --keep-ownership option to add/copy commands to keep file ownership.

[r10r]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This new flag is required if you want to keep file ownership when using buildah copy or buildah add to copy local
content into a container.

How to verify it

Copy a single file:

c=$(buildah from alpine)
buildah run $c chown 1111:2222 /bin/busybox
buildah run $c ls -l /bin/busybox

c1=$(buildah from alpine)
buildah copy --from=$c $c1 /bin/busybox .
buildah run $c1 ls -l busybox
buildah copy --keep-ownership --from=$c $c1 /bin/busybox .
buildah run $c1 ls -l busybox

expected output

-rwxr-xr-x 1 666 333 824984 Nov 23 00:57 /bin/busybox
5d08f12cf41e4b182160b5be74ec4ff70501725020c93033377d49ee819013e1
-rwxr-xr-x 1 root root 824984 Nov 23 00:57 busybox
aa397c0a925a45d0673c1184522da2ae4e452695ee52dbf316b142db23792bee
-rwxr-xr-x 1 666 333 824984 Nov 23 00:57 busybox
8804e0c69493d9c20eff48fcfec7d7774cb18589aefb3ef3862c390f4e282c82
615bc8c131cfd9248869303d9831839486311a565f13681ba9d8991dcd431e9c

Copy a folder.

c=$(buildah from alpine)
buildah run --workingdir /work $c touch foo bar
buildah run --workingdir /work $c chown 1111:2222 foo
buildah run --workingdir /work $c chown 3333:4444 bar
buildah run --workingdir /work $c ls -l /work

c1=$(buildah from alpine)
buildah copy --from=$c $c1 /work /work
buildah run $c1 ls -l /work
buildah copy --keep-ownership --from=$c $c1 /work /work-keep
buildah run $c1 ls -l /work-keep

expected output

total 0
-rw-r--r--    1 3333     4444             0 Feb  3 16:29 bar
-rw-r--r--    1 1111     2222             0 Feb  3 16:29 foo
9ae71c065789d5c930057e399a3e74a5fabb7f5fd177be91287df2d8c98e3700
total 0
-rw-r--r--    1 root     root             0 Feb  3 16:29 bar
-rw-r--r--    1 root     root             0 Feb  3 16:29 foo
381fc727b2f884b268a3ac47c62cacdbb7edf4f598079e966ec43c5c29eab793
total 0
-rw-r--r--    1 3333     4444             0 Feb  3 16:29 bar
-rw-r--r--    1 1111     2222             0 Feb  3 16:29 foo
8446780cb62256c465c0c8d1282a112ef2a591836741d212c6154c6d795937d4
e74373ee07b5ccf717254b8c4c024d643449875afcaf8e05ca991f5f8ce0a9ed

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Tests and documentation follow.

Does this PR introduce a user-facing change?

Added flag `--keep-ownership` to `add` and `copy` commands. If the flag is set the file
ownership is preserved when adding/copying local files to a container.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: r10r
To complete the pull request process, please assign cevich after the PR has been reviewed.
You can assign the PR to them by writing /assign @cevich in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[r10r]

Please confirm if this can be added - I'll then add tests and documentation as soon as possible.

[r10r]

I dropped the commits which are not required. Simply exposing KeepOwnership is enough.
@TomSweeneyRedHat Sorry for the extra work.

[rhatdan]

You need to add a test to get this in.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[rhatdan]

@r10r Still working on this?

[r10r]

@rhatdan Yes, I think I'll get it done this week.

[Dexus]

@r10r Would love to see this integrated. Can you please finish it? Thank you!

[Dexus]

@flouthoc @rhatdan

I tried to add the test, but I get stopped from the Vendor package: https://github.com/openshift/imagebuilder/blob/master/dispatchers.go#L199

What would be the better approach to add this feature? Create a PR to the vendor or have a fork/mirror with the changes and replace the vendor package?

[flouthoc]

@Dexus You can replace imagebuilder with your own fork for testing and in the PR as well but before the PR is merged that fork has to point to actual upstream so changes has to go into upstream imagebuilder as well.

[r10r]

@Dexus Thanks for looking at the tests. It seems there is missing some documentation right now for the checks to continue

[+0149s] ./tests/validate/whitespace.sh
[+0150s] ./hack/xref-helpmsgs-manpages
[+0151s] xref-helpmsgs-manpages: buildah add --help lists --keep-ownership, but --keep-ownership not in docs/buildah-add.1.md
[+0151s] xref-helpmsgs-manpages: buildah copy --help lists --keep-ownership, but --keep-ownership not in docs/buildah-copy.1.md
[+0151s] make: *** [Makefile:114: validate] Error 1

[Dexus]

looks like it will need much more then only this small PR. In the test it will not copy the uid/gid of the file. While it is copy the uid/gid from other images/multi step builds - but this is also by default.

@Dexus Thanks for looking at the tests. It seems there is missing some documentation right now for the checks to continue

[+0149s] ./tests/validate/whitespace.sh
[+0150s] ./hack/xref-helpmsgs-manpages
[+0151s] xref-helpmsgs-manpages: buildah add --help lists --keep-ownership, but --keep-ownership not in docs/buildah-add.1.md
[+0151s] xref-helpmsgs-manpages: buildah copy --help lists --keep-ownership, but --keep-ownership not in docs/buildah-copy.1.md
[+0151s] make: *** [Makefile:114: validate] Error 1

Sure, but I'm currently first on the stage where I would like to make the tests working. I'm not getting problems with the docs currently :)

[Dexus]

Okay, I'm on a good way openshift/imagebuilder#227 is created and will updated once I'm having a green lights from tests.

[Dexus]

@r10r I'm not able to create a PR for your fork repository, is it okay, when I create a new PR or will you merge my repository into yours?

https://github.com/Dexus-Forks/buildah/tree/copy-keep-ownership

⚠️ Notice:

go.mod needs to add a replace for github.com/openshift/imagebuilder as long openshift/imagebuilder#227 is unmerged

go mod edit -replace github.com/openshift/imagebuilder=github.com/Dexus-Forks/imagebuilder

[r10r]

@Dexus I'm glad to see you pushing it forward.

Closing in favour of #4001