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

fix: file ownership of files copied into the container #2006

Closed
wants to merge 4 commits into from

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Sep 14, 2024

fix file ownership of files copied into the container

Use docker cp -a to copy files into the container.
This requires to create the container with --user for the command not to fail.

fix #2004
based on #2005

Use `docker cp -a` to copy files in/out of the container.
This requires to create the container with `--user` for the command not to fail.
@mayeut
Copy link
Member Author

mayeut commented Sep 14, 2024

Related issues in docker:
moby/moby#45793
moby/moby#41727

There's also the fact that when using an image not specifying a user, it does not fallback to root as it should (hence the added --user).

For the --user arg added, it could conflict with the default user in custom images or an engine config option set by the cibuildwheel user so, it would in fact probably require:

  • checking wether or not the engine config sets this
  • if not, checking wether or not the image sets this (pull image and inspect --format '{{ .Config.User }}')
  • if not, add --user

Even if doing this, docker might fix the reversed behavior of -a which would break again cibuildwheel depending on docker version.

Given all that I'm not sure whether or not we should just reverse to using tar ?

@mayeut mayeut changed the title fix: file ownership of files copied into/from the container fix: file ownership of files copied intothe container Sep 15, 2024
@mayeut mayeut changed the title fix: file ownership of files copied intothe container fix: file ownership of files copied into the container Sep 15, 2024
@joerick
Copy link
Contributor

joerick commented Sep 15, 2024

Thanks for pulling these options together. I meant to check the permissions thing with the docker cp PR and I confess I forgot.

It's crazy to me that docker cp would retain UIDs by default. I suppose most users run their containers as root and therefore don't see the issue.

Regarding adding --user as an option into oci_container.py, the only place I've seen this mentioned is this discussion thread, where a user wanted to run their container as a different user, because there was some script in their build complaining about running as root. That issue wasn't solved using --user 1001 because oci_container.py writes to some locations like /project and /constraints.txt that wouldn't be writable by any user other than root. So I think that it's very unlikely we have any instances of users running their containers with the primary user as anything other than root.

Even if doing this, docker might fix the reversed behavior of -a which would break again cibuildwheel depending on docker version.

I'm very confused by this... according to the documentation by Docker:

The cp command behaves like the Unix cp -a command in that directories are copied recursively with permissions preserved if possible. Ownership is set to the user and primary group at the destination. For example, files copied to a container are created with UID:GID of the root user. Files copied to the local machine are created with the UID:GID of the user which invoked the docker cp command. However, if you specify the -a option, docker cp sets the ownership to the user and primary group at the source.

By my reading, this paragraph contradicts itself "copied recursively with permissions preserved if possible" versus "Ownership is set to the user and primary group at the destination".

So yes, this does seem like it's not set in stone. Having said that, I can't imagine they would change the de facto behaviour because it would break so many workflows. More likely, they'll fix the documentation.

@joerick
Copy link
Contributor

joerick commented Sep 15, 2024

I definitely think, regardless of how we get there, that files in the container should be owned by the primary user, and once copied out of the container they should be owned by the user running cibuildwheel.

I'm slightly torn on these options, right now, given the strangeness of the docker cp, regretfully I think I am more inclined to go back to the tar pipes setup we had before. Where is your preference @mayeut ?

@mayeut
Copy link
Member Author

mayeut commented Sep 15, 2024

I'm slightly torn on these options, right now, given the strangeness of the docker cp, regretfully I think I am more inclined to go back to the tar pipes setup we had before. Where is your preference mayeut ?

I also think that going back to the tar pipes setup we had before is what we should do for now.
Just as a side note, I did not restore the tar pipe in copy_out as docker cp works properly in this case (with or without -a) in #2007.
I will close this PR in favor of #2007

@mayeut mayeut closed this Sep 15, 2024
@mayeut mayeut deleted the check-ownership branch September 16, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File ownership not preserved when copying the project inside the container
2 participants