-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add U volume flag to chown source volumes #8349
Add U volume flag to chown source volumes #8349
Conversation
295a7b7
to
b1aebda
Compare
There is some fast chown code available in |
@rhatdan I am currently getting a CI issue because I have modified a vendor file (vendor/github.com/containers/buildah/pkg/parse/parse.go). It says I'll take a look at the chown code you provided. Thanks |
Submit a PR of the change to Buildah, and then vendor the updated Buildah into Podman in this PR. |
Waiting for a new release of Buildah to met the dependencies for this PR. |
You do not need to wait for a release. You can just update the vendor of buildah here, based on major. We will update the version of buildah before releasing a new version of podman. |
a4b5456
to
e942efd
Compare
@EduardoVega is this still a WIP pr? |
@rhatdan I think it is not since all buildah prerequisites were already present in podman. |
is there anything else I need to do ? or just wait until the PR is reviewed |
Buildah should be released tomorrow, so we can move this ahead. |
libpod/container_internal_linux.go
Outdated
@@ -2116,6 +2140,39 @@ func (c *Container) copyOwnerAndPerms(source, dest string) error { | |||
return nil | |||
} | |||
|
|||
// chownSourceVolume changes the ownership of a source volume. | |||
func (c *Container) chownSourceVolume(path string, UID, GID int) error { |
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.
Not blocking for this PR, but we really should break this code out into a shared source with Buildah. Should optimize for multi-threads like has been done for SELinux as well. This procedure could take a long time.
@EduardoVega We would really to get this in by Friday's feature freeze. |
e942efd
to
fdf7b55
Compare
This is not going to make the 3.0 release, Will have to wait until 3.1 |
fdf7b55
to
5773b95
Compare
@EduardoVega Is this still a WIP or do you want it reviewed? |
@rhatdan This is ready to be reviewed. |
pkg/util/mountOpts.go
Outdated
@@ -112,6 +112,14 @@ func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string | |||
return nil, errors.Wrapf(ErrDupeMntOption, "only one of 'z' and 'Z' can be used") | |||
} | |||
foundZ = true | |||
case "U": | |||
if isTmpfs { | |||
return nil, errors.Wrapf(ErrBadMntOption, "the 'U' option is not allowed with tmpfs mounts") |
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.
Why Not? Does this happen automatically? IE If I am running as a user, do the tmpfs get mounted as owned by that user?
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.
This was not happening automatically. To add correct ownership the uid and gid will be added to the mount options when the U flag is specified
test/e2e/run_volume_test.go
Outdated
Skip("cannot find mappings for the current user") | ||
} | ||
|
||
SkipIfRemote("Overlay volumes only work locally") |
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.
Move to the top
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.
Done
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EduardoVega, rhatdan 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:
Approvers can indicate their approval by writing |
e2864d0
to
8b39ee1
Compare
6d59fc9
to
001afb6
Compare
Thanks @EduardoVega This is a nice new feature. |
@mheon @vrothberg PTAL |
@EduardoVega We would love to have you present and demonstrate this at a monthly Podman users group, or write a blog on this feature. |
libpod/container.go
Outdated
@@ -236,6 +236,8 @@ type ContainerOverlayVolume struct { | |||
Dest string `json:"dest"` | |||
// Source specifies the source path of the mount. | |||
Source string `json:"source,omitempty"` | |||
// Chown changes the source UID and GID ownership. | |||
Chown bool `json:"chown,omitempty"` |
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.
Can you make this a more generic Options []string
instead? It seems like we will have more of these later on, and the other volume types don't have per-option bools.
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.
done
pkg/util/utils.go
Outdated
@@ -692,3 +693,16 @@ func CoresToPeriodAndQuota(cores float64) (uint64, int64) { | |||
func PeriodAndQuotaToCores(period uint64, quota int64) float64 { | |||
return float64(quota) / float64(period) | |||
} | |||
|
|||
// IdtoolsToRuntimeSpec converts idtools ID mapping to the one of the runtime spec. | |||
func IdtoolsToRuntimeSpec(idMaps []idtools.IDMap) (convertedIDMap []specs.LinuxIDMapping) { |
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.
Suggest renaming to IDtoolsToRuntimeSpec
(Capitalize the first D) - I think our linter checks for this one now. Might be it's not linting this directory?
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.
Thanks and done
Two suggestions, but on the whole this LGTM |
Signed-off-by: Eduardo Vega <edvegavalerio@gmail.com>
001afb6
to
874f232
Compare
@rhatdan thanks and I think I could present it and also create a blog about this. I might need more details but I would love to do it. |
/lgtm |
@EduardoVega very nice work, ty! I too would love to have you present at a Podman community meeting. They are on the first Tuesday of the month. Next week is filled up, but the April one I'd love to have you present at. However, we're holding that one later in the day than usual, 8:00 p.m. Eastern so the folks in China can attend. So if you wanted to wait until the one in May which would be at 11:00 a.m. Eastern, that would work too. |
@TomSweeneyRedHat April one should work. I'm on Central America Time which is 1 hour behind of Eastern |
@EduardoVega Awesome! Would you mind sending me an e-mail (tsweeney@redhat.com) so I can touch base with you in mid-March with meeting info? |
@TomSweeneyRedHat sure. |
This will ensure that the uid,gid will be the same between the containers and volumes. The source volume will be chowned if the flag is specified to match the uid, gid of the container.
Fixes #7778
Signed-off-by: Eduardo Vega edvegavalerio@gmail.com