Revert "Fix copyUIDGID parameter inversion in Docker compat API"

[matejvasek]

This reverts commit 2b848cc.

The commit's assumption that copyUIDGID=true means that UID/GID from the tar should be preserved does not seem to be true. Opposite is true: if copyUIDGUID=true the main UID/GID of the container is used. I tested this on moby.

Also brief look at the source code seems to confirm this:
https://github.com/moby/moby/blob/16880e9e1ba21593215b40241d62c043410e0ae7/daemon/archive_unix.go#L136-L144
https://github.com/moby/moby/blob/16880e9e1ba21593215b40241d62c043410e0ae7/daemon/archive_tarcopyoptions_unix.go#L22-L30

Yes it's quite contra-intuitive but it does behave that way.

See this issue opened since 2017 moby/moby#34096

Now, docker cp hostfile container:containerpath will copy a file from the host to the container but preserving the original uid/gid (with the respective mapping to the container). docker cp -a hostfile container:containerpath (as defined in the documentation) is meant to "copy all uid/gid information" but it is in fact setting the uid and gid to the main user inside the container.

Also apparently there was a PR to fix this moby/moby#34099 but is has newer been merged.

And another bug report docker/docs#17533 that suggest to change in documentation instead of the behaviour.

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

• Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
  commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
  the sign-off email address. See CONTRIBUTING.md
  for more information.
• Referenced issues using Fixes: #00000 in commit message (if applicable)
• Tests have been added/updated (or no tests are needed)
• Documentation has been updated (or no documentation changes are needed)
• All commits pass make validatepr (format/lint checks)
• Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

[matejvasek]

/cc @giuseppe @Honny1

[matejvasek]

/cc @dwaynebradley

[openshift-ci]

@matejvasek: GitHub didn't allow me to request PR reviews from the following users: dwaynebradley.

Note that only containers members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @dwaynebradley

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[matejvasek]

I assume this does not need release notes, since the reverted commit has not been added to any release branch yet.

[matejvasek]

/cc @TomSweeneyRedHat

[Honny1]

I am not sure what to do about this. The Docker documentation says one thing, but the software actually does another. Breaking other tools is also not good.

Docker rest API reference

[Luap99]

I am not sure what to do about this. The Docker documentation says one thing, but the software actually does another. Breaking other tools is also not good.

Well the compat API is supposed to be docker compatible, so it should do what docker does not what docker docs say it does. As a general rule if there are users depending on the behavior then it is much easier to keep such behavior working and update the docs instead of breaking the users, especially since there would be no good way for them to handle this because the option suddenly does the opposite so they would need to version check and invert the param which is just ugly as hell.

[Luap99]

LGTM, but as general rule can you add the context of these things also in the commit message please. I prefer to read the git log with context and not have to go to github for every commit to figure out why it was done.

[matejvasek]

moby/moby#51523

[matejvasek]

@Luap99 I could force push amended message if you wish.

[matejvasek]

I guess the reverted commit has not been merged to any of the release branches, right?

[matejvasek]

When it comes to cherry-picking to releases, is there somebody to we should notify to not include the reverted commit in backports?

[giuseppe]

LGTM

[Honny1]

LGTM. Let's await the Moby maintainers' response in the discussion. I will then update the documentation based on their answer.

[Luap99]

When it comes to cherry-picking to releases, is there somebody to we should notify to not include the reverted commit in backports?

We don't have a process for that. Your comments on the original PR should be enough for others looking at it to not backport it I suppose

[Luap99]

/lgtm

[matejvasek]

OK tried to look on the original issue #27332 and talk to the reporter of it.

It appears that he did really observe inconsistency with docker behaviour.
But the difference was not the parameter inversion.

The flag works same way on podman/docker, with exception when used with a container with no explicit user.
In docker the flag/param is ineffectual for such a containers and uid/gid form tar are always used.
The podman simply assumes root user and acts accordingly.

See #27538

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99, matejvasek

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99,giuseppe]

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