Add U volume flag to chown source volumes #8349

EduardoVega · 2020-11-16T02:11:38Z

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

rhatdan · 2020-11-16T14:53:54Z

There is some fast chown code available in containers/storage/drivers/chown that can do a recursive chown with multiple threads, I believe. We might want to take advantage of this.

EduardoVega · 2020-11-16T17:17:36Z

@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 please sync the vendor.conf and commit all changes, what's the correct way of doing this change?

I'll take a look at the chown code you provided.

Thanks

mheon · 2020-11-16T17:36:32Z

Submit a PR of the change to Buildah, and then vendor the updated Buildah into Podman in this PR.

EduardoVega · 2021-01-04T13:44:54Z

Waiting for a new release of Buildah to met the dependencies for this PR.

rhatdan · 2021-01-04T16:29:38Z

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.

rhatdan · 2021-01-07T11:35:49Z

@EduardoVega is this still a WIP pr?

EduardoVega · 2021-01-07T11:46:10Z

@rhatdan I think it is not since all buildah prerequisites were already present in podman.

EduardoVega · 2021-01-09T13:04:56Z

is there anything else I need to do ? or just wait until the PR is reviewed

rhatdan · 2021-01-10T10:10:54Z

Buildah should be released tomorrow, so we can move this ahead.

docs/source/markdown/podman-run.1.md

rhatdan · 2021-01-10T10:16:12Z

libpod/container_internal_linux.go

@@ -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 {


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.

libpod/container_internal_linux.go

rhatdan · 2021-01-12T12:54:24Z

@EduardoVega We would really to get this in by Friday's feature freeze.

rhatdan · 2021-01-14T18:29:50Z

This is not going to make the 3.0 release, Will have to wait until 3.1

rhatdan · 2021-01-15T08:54:08Z

@EduardoVega Is this still a WIP or do you want it reviewed?

EduardoVega · 2021-01-15T13:37:43Z

@rhatdan This is ready to be reviewed.

libpod/container_internal_linux.go

rhatdan · 2021-01-15T13:57:37Z

pkg/util/mountOpts.go

@@ -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")


Why Not? Does this happen automatically? IE If I am running as a user, do the tmpfs get mounted as owned by that user?

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

rhatdan · 2021-01-15T13:58:11Z

test/e2e/run_volume_test.go

+			Skip("cannot find mappings for the current user")
+		}
+
+		SkipIfRemote("Overlay volumes only work locally")


Move to the top

openshift-ci-robot · 2021-01-15T13:58:57Z

[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:

~~OWNERS~~ [rhatdan]

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

rhatdan · 2021-02-22T11:04:55Z

Thanks @EduardoVega This is a nice new feature.
LGTM
@containers/podman-maintainers PTAL

rhatdan · 2021-02-22T11:05:09Z

@mheon @vrothberg PTAL

rhatdan · 2021-02-22T11:07:04Z

@EduardoVega We would love to have you present and demonstrate this at a monthly Podman users group, or write a blog on this feature.

mheon · 2021-02-22T14:49:20Z

libpod/container.go

@@ -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"`


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.

mheon · 2021-02-22T14:51:01Z

pkg/util/utils.go

@@ -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) {


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?

Thanks and done

mheon · 2021-02-22T14:51:22Z

Two suggestions, but on the whole this LGTM

Signed-off-by: Eduardo Vega <edvegavalerio@gmail.com>

EduardoVega · 2021-02-23T14:54:57Z

@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.

mheon · 2021-02-23T15:11:04Z

/lgtm
Nice work @EduardoVega

TomSweeneyRedHat · 2021-02-23T15:50:20Z

@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.

EduardoVega · 2021-02-23T16:13:49Z

@TomSweeneyRedHat April one should work. I'm on Central America Time which is 1 hour behind of Eastern

TomSweeneyRedHat · 2021-02-23T22:19:06Z

@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?

EduardoVega · 2021-02-23T22:21:23Z

@TomSweeneyRedHat sure.

EduardoVega force-pushed the 7778-chowning-based-on-uid branch from 295a7b7 to b1aebda Compare November 16, 2020 14:52

rhatdan changed the title ~~Add U volume flag to chown source volumes based on container's uid,gid~~ [WIP] Add U volume flag to chown source volumes based on container's uid,gid Nov 17, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2020

EduardoVega changed the title ~~[WIP] Add U volume flag to chown source volumes based on container's uid,gid~~ [WIP] Add U volume flag to chown source volumes Dec 17, 2020

EduardoVega force-pushed the 7778-chowning-based-on-uid branch 2 times, most recently from a4b5456 to e942efd Compare January 7, 2021 01:52

rhatdan approved these changes Jan 10, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2021

rhatdan added the 3.0 Features label Jan 12, 2021

EduardoVega force-pushed the 7778-chowning-based-on-uid branch from e942efd to fdf7b55 Compare January 13, 2021 01:23

rhatdan removed the 3.0 Features label Jan 14, 2021

EduardoVega force-pushed the 7778-chowning-based-on-uid branch from fdf7b55 to 5773b95 Compare January 14, 2021 21:55

rhatdan changed the title ~~[WIP] Add U volume flag to chown source volumes~~ Add U volume flag to chown source volumes Jan 15, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2021

rhatdan approved these changes Jan 15, 2021

View reviewed changes

EduardoVega mentioned this pull request Jan 24, 2021

Add U volume flag to chown source volumes containers/common#420

Merged

EduardoVega mentioned this pull request Jan 25, 2021

Reuse chown function from containers/common when U volume flag is used containers/buildah#2931

Merged

EduardoVega force-pushed the 7778-chowning-based-on-uid branch 2 times, most recently from e2864d0 to 8b39ee1 Compare February 4, 2021 01:49

EduardoVega force-pushed the 7778-chowning-based-on-uid branch 3 times, most recently from 6d59fc9 to 001afb6 Compare February 16, 2021 15:58

EduardoVega mentioned this pull request Feb 16, 2021

Option to use UIDs only once #7778

Closed

mheon reviewed Feb 22, 2021

View reviewed changes

Add U volume flag to chown source volumes

874f232

Signed-off-by: Eduardo Vega <edvegavalerio@gmail.com>

EduardoVega force-pushed the 7778-chowning-based-on-uid branch from 001afb6 to 874f232 Compare February 23, 2021 04:55

openshift-ci-robot assigned mheon Feb 23, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2021

openshift-merge-robot merged commit 1702cbc into containers:master Feb 23, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add U volume flag to chown source volumes #8349

Add U volume flag to chown source volumes #8349

EduardoVega commented Nov 16, 2020

rhatdan commented Nov 16, 2020

EduardoVega commented Nov 16, 2020 •

edited

Loading

mheon commented Nov 16, 2020

EduardoVega commented Jan 4, 2021

rhatdan commented Jan 4, 2021

rhatdan commented Jan 7, 2021

EduardoVega commented Jan 7, 2021

EduardoVega commented Jan 9, 2021

rhatdan commented Jan 10, 2021

rhatdan Jan 10, 2021

rhatdan commented Jan 12, 2021

rhatdan commented Jan 14, 2021

rhatdan commented Jan 15, 2021

EduardoVega commented Jan 15, 2021

rhatdan Jan 15, 2021

EduardoVega Feb 4, 2021

rhatdan Jan 15, 2021

EduardoVega Feb 4, 2021

openshift-ci-robot commented Jan 15, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

mheon Feb 22, 2021

EduardoVega Feb 23, 2021

mheon Feb 22, 2021

EduardoVega Feb 23, 2021

mheon commented Feb 22, 2021

EduardoVega commented Feb 23, 2021

mheon commented Feb 23, 2021

TomSweeneyRedHat commented Feb 23, 2021

EduardoVega commented Feb 23, 2021

TomSweeneyRedHat commented Feb 23, 2021

EduardoVega commented Feb 23, 2021

Add U volume flag to chown source volumes #8349

Add U volume flag to chown source volumes #8349

Conversation

EduardoVega commented Nov 16, 2020

rhatdan commented Nov 16, 2020

EduardoVega commented Nov 16, 2020 • edited Loading

mheon commented Nov 16, 2020

EduardoVega commented Jan 4, 2021

rhatdan commented Jan 4, 2021

rhatdan commented Jan 7, 2021

EduardoVega commented Jan 7, 2021

EduardoVega commented Jan 9, 2021

rhatdan commented Jan 10, 2021

Choose a reason for hiding this comment

rhatdan commented Jan 12, 2021

rhatdan commented Jan 14, 2021

rhatdan commented Jan 15, 2021

EduardoVega commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 15, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Feb 22, 2021

EduardoVega commented Feb 23, 2021

mheon commented Feb 23, 2021

TomSweeneyRedHat commented Feb 23, 2021

EduardoVega commented Feb 23, 2021

TomSweeneyRedHat commented Feb 23, 2021

EduardoVega commented Feb 23, 2021

EduardoVega commented Nov 16, 2020 •

edited

Loading