-
Notifications
You must be signed in to change notification settings - Fork 103
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
package-builder: add podman support via --container_tool #1722
base: main
Are you sure you want to change the base?
Conversation
"-v", fmt.Sprintf("%s:/pkgsrc", po.Root), | ||
"-v", fmt.Sprintf("%s:/pkgscripts", po.Scripts), | ||
"-v", fmt.Sprintf("%s:/out", outputPathDir), | ||
"-v", fmt.Sprintf("%s:/pkgsrc:Z", po.Root), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... Docker has several warnings about this:
If you use selinux you can add the z or Z options to modify the selinux label of the host file or directory being mounted into the container. This affects the file or directory on the host machine itself and can have consequences outside of the scope of Docker.
- The z option indicates that the bind mount content is shared among multiple containers.
- The Z option indicates that the bind mount content is private and unshared.
Use extreme caution with these options. Bind-mounting a system directory such as /home or /usr with the Z option renders your host machine inoperable and you may need to relabel the host machine files by hand.
I think I understand the why of this, but it makes me kinda unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. My interpretation of the warning is: don't attach important system directories as Docker volumes as it can allow the docker container to manipulate the data within the volume.
If it makes you feel better, I can add the :Z
flag only if --container_tool=podman
. The reason this works in Docker is that by default, Docker runs as root. podman normally executes as a regular user (rootless mode). Docker also has a rootless mode, but nobody uses it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was also my interpretation. And also that this would only have negative ramifications if the build wasn't on an ephemeral machine.
I think if we conditionalize it to just podman it's a lot safer and more narrow.
@@ -54,6 +54,11 @@ func runMake(args []string) error { | |||
false, | |||
"enable debug logging", | |||
) | |||
flContainerTool = flagset.String( | |||
"container_tool", | |||
"docker", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick note for the team that if we merge this, we'll need to update our internal tooling to provide this default value since we don't call package-builder
on the command line.
This allows the launcher to be built on Fedora, which ships with a working podman environment that runs as a regular user (no background daemon required). In comparison, setting up Docker is a pain and gives the user root-level privilege.
This PR does add the ":Z" flag to container volume mounts, so that the volume permissions allow access from the host filesystem, which the build process assumes. Without this flag, rootless podman encounters an error:
The ":z" mount flag also works, but has a looser security model. This flag is supported by Docker and Podman.
This PR also changes the container pull location from an ambiguous "kolide/fpm:latest" to the absolute path of ""docker.io/kolide/fpm:latest". This is necessary as Podman is not pre-programmed to pull from "docker.io" by default.