Add --umask flag for create, run #7006

ashley-cui · 2020-07-17T13:05:38Z

--umask sets the umask inside the container
Defaults to 0022

Replaces #6946

Co-authored-by: Daniel J Walsh [email protected]
Signed-off-by: Ashley Cui [email protected]

TomSweeneyRedHat · 2020-07-17T13:45:19Z

test/e2e/create_test.go

+		session = podmanTest.Podman([]string{"create", "--umask", "9999", "--name", "bad", ALPINE})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Not(Equal(0)))
+	})


You're testing both create and confirming with inspect in here which is good. It might make sense to also add a part of this to the inspect test too, even though it would be redundant. I'll let @mheon make the call there.

TomSweeneyRedHat · 2020-07-17T13:46:00Z

All kinds of test unhappiness @ashley-cui

rhatdan · 2020-07-17T20:20:26Z

The random tests are blowing up with
time="2020-07-17T16:18:53-04:00" level=error msg="error starting some container dependencies"
time="2020-07-17T16:18:53-04:00" level=error msg=""Invalid Umask Value: strconv.ParseUint: parsing \"\": invalid syntax""

rhatdan · 2020-07-17T20:32:45Z

This looks like there is something wrong with the runtime-tools vendor, that is causing this issue.

# ./bin/podman pod create -p 8080:80 --name test1
d164202b886bb247f5b32b36ba0d6aa92aa14ab050a09eadc2fde0875cf1d13d
# ./bin/podman run --pod test1 fedora echo top
Error: open `/proc/1513113/ns/net`: No such file or directory: OCI runtime command not found error

@giuseppe @mheon PTAL

rhatdan · 2020-07-17T20:38:22Z

@ashley-cui I think you might want

diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 53b7f31c4..ed0a0f2da 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -341,12 +341,13 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
                g.SetProcessGID(uint32(execUser.Gid))
        }
 
-       decVal, err := strconv.ParseUint(c.config.Umask, 8, 32)
-       if err != nil {
-               return nil, errors.Wrapf(err, "Invalid Umask Value")
+       if c.config.Umask != "" {
+               decVal, err := strconv.ParseUint(c.config.Umask, 8, 32)
+               if err != nil {
+                       return nil, errors.Wrapf(err, "Invalid Umask Value: %q", c.config.Umask)
+               }
+               g.SetProcessUmask(uint32(decVal))
        }
-       g.SetProcessUmask(uint32(decVal))
-
        // Add addition groups if c.config.GroupAdd is not empty
        if len(c.config.Groups) > 0 {
                gids, err := lookup.GetContainerGroups(c.config.Groups, c.state.Mountpoint, overrides)

ashley-cui · 2020-07-17T20:40:21Z

@rhatdan already updated :) the runtime-tools i vendored in myself, so it might be a vendoring error caused by me..

mheon · 2020-07-17T22:19:16Z

libpod/container.go

@@ -435,6 +435,9 @@ type ContainerConfig struct {
 	// Timezone is the timezone inside the container.
 	// Local means it has the same timezone as the host machine
 	Timezone string `json:"timezone,omitempty"`
+
+	// Umask is the umask inside the container.
+	Umask string `json:"umask,omitempty"`


Why are we storing this as a string? We should use whatever type the OCI spec uses - probably an int? Parsing should be done when we set the umask.

The value is actually an octal. Storing it as an int, may make it hard to convert and hard to view in inspect.

yep, it's a octal representing a bitmap. setting umask to 0022 would actually show up as 18 if stored as an int. i got stuck on this for a good hour or two when implementing LOL

rhatdan · 2020-07-18T10:14:00Z

test/e2e/create_test.go

+		inspect.WaitWithDefaultTimeout()
+		data := inspect.InspectContainerToJSON()
+		Expect(len(data)).To(Equal(1))
+		Expect(data[0].Config.Umask).To(Equal("0022"))


Coud you add a test for

podman run --umask 0077 fedora umask
And make sure it outputs 0077

ashley-cui · 2020-07-20T14:10:35Z

I'm unable to reproduce locally the system tests that are failing but I confirmed that umask doesn't work on ubuntu at all. Any pointers on how to proceed? @rhatdan

edsantiago · 2020-07-20T15:41:27Z

The system-test failures look suspiciously like something I may have introduced in #6958 - probably a bad interaction with a missing-cleanup bug in one of the integration tests. PR in progress.

The umask-on-ubuntu issue looks real though. FW(little)IW my guess is it's runc vs crun.

ashley-cui · 2020-07-20T15:45:38Z

@edsantiago thanks! any idea who i should talk to about the ubuntu stuff?

edsantiago · 2020-07-20T16:17:23Z

#7026 should fix the system-test flake. I'm sorry, I have no experience with Ubuntu and no idea how to investigate there. If you want to look into crun vs runc, you can reboot a Fedora system into cgroups v1 mode by booting with systemd.unified_cgroup_hierarchy=0

edsantiago · 2020-07-21T17:48:33Z

test/e2e/run_test.go

+
+		session = podmanTest.Podman([]string{"run", "--umask", "9999", "--rm", ALPINE, "sh", "-c", "umask"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Not(Equal(0)))


Ed's standard rant: it's important to check error messages, not just error codes:

Expect(session.ErrorToString()).To(ContainSubstring("Invalid umask"))

Without that, the test could be failing for infinite unrelated reasons: too often a misspelled option or command name yields the expected nonzero exit status, but the test isn't actually testing anything.

Aside from that, lgtm. Nice set of tests.

Sounds good, will keep in mind for future tests too :)

--umask sets the umask inside the container Defaults to 0022 Co-authored-by: Daniel J Walsh <[email protected]> Signed-off-by: Ashley Cui <[email protected]>

giuseppe

LGTM

openshift-ci-robot · 2020-07-22T06:46:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, giuseppe

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~~ [giuseppe]

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

rhatdan · 2020-07-22T09:32:37Z

/lgtm

openshift-ci-robot requested review from jwhonce and rhatdan July 17, 2020 13:05

ashley-cui force-pushed the umask branch 2 times, most recently from 49ebbe8 to 0c29264 Compare July 17, 2020 13:20

ashley-cui mentioned this pull request Jul 17, 2020

[WIP] Add support for Umask #6946

Closed

TomSweeneyRedHat reviewed Jul 17, 2020

View reviewed changes

ashley-cui force-pushed the umask branch 2 times, most recently from 6dbb789 to 42bef7c Compare July 17, 2020 17:35

ashley-cui force-pushed the umask branch from 42bef7c to 35f5988 Compare July 17, 2020 20:30

mheon reviewed Jul 17, 2020

View reviewed changes

rhatdan reviewed Jul 18, 2020

View reviewed changes

ashley-cui force-pushed the umask branch 3 times, most recently from ac042a1 to 6caa6c1 Compare July 19, 2020 01:18

ashley-cui force-pushed the umask branch 2 times, most recently from ce51a2e to c6bb490 Compare July 21, 2020 17:26

edsantiago reviewed Jul 21, 2020

View reviewed changes

Add --umask flag for create, run

d4d3fbc

--umask sets the umask inside the container Defaults to 0022 Co-authored-by: Daniel J Walsh <[email protected]> Signed-off-by: Ashley Cui <[email protected]>

ashley-cui force-pushed the umask branch from c6bb490 to d4d3fbc Compare July 21, 2020 18:22

giuseppe approved these changes Jul 22, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2020

openshift-ci-robot assigned rhatdan Jul 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020

openshift-merge-robot merged commit 59bad8b into containers:master Jul 22, 2020

ashley-cui deleted the umask branch October 12, 2020 13:59

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 24, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --umask flag for create, run #7006

Add --umask flag for create, run #7006

ashley-cui commented Jul 17, 2020 •

edited

Loading

TomSweeneyRedHat Jul 17, 2020

TomSweeneyRedHat commented Jul 17, 2020

rhatdan commented Jul 17, 2020

rhatdan commented Jul 17, 2020

rhatdan commented Jul 17, 2020

ashley-cui commented Jul 17, 2020

mheon Jul 17, 2020

rhatdan Jul 18, 2020

ashley-cui Jul 19, 2020 •

edited

Loading

rhatdan Jul 18, 2020

ashley-cui commented Jul 20, 2020

edsantiago commented Jul 20, 2020

ashley-cui commented Jul 20, 2020

edsantiago commented Jul 20, 2020

edsantiago Jul 21, 2020

ashley-cui Jul 21, 2020

giuseppe left a comment

openshift-ci-robot commented Jul 22, 2020

rhatdan commented Jul 22, 2020

Add --umask flag for create, run #7006

Add --umask flag for create, run #7006

Conversation

ashley-cui commented Jul 17, 2020 • edited Loading

TomSweeneyRedHat Jul 17, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jul 17, 2020

rhatdan commented Jul 17, 2020

rhatdan commented Jul 17, 2020

rhatdan commented Jul 17, 2020

ashley-cui commented Jul 17, 2020

mheon Jul 17, 2020

Choose a reason for hiding this comment

rhatdan Jul 18, 2020

Choose a reason for hiding this comment

ashley-cui Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

rhatdan Jul 18, 2020

Choose a reason for hiding this comment

ashley-cui commented Jul 20, 2020

edsantiago commented Jul 20, 2020

ashley-cui commented Jul 20, 2020

edsantiago commented Jul 20, 2020

edsantiago Jul 21, 2020

Choose a reason for hiding this comment

ashley-cui Jul 21, 2020

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 22, 2020

rhatdan commented Jul 22, 2020

ashley-cui commented Jul 17, 2020 •

edited

Loading

ashley-cui Jul 19, 2020 •

edited

Loading