-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 --umask flag for create, run #7006
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,3 +50,5 @@ dns_servers=[ "1.2.3.4", ] | |
dns_options=[ "debug", ] | ||
|
||
tz = "Pacific/Honolulu" | ||
|
||
umask = "0002" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"io/ioutil" | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
|
||
. "github.com/containers/libpod/v2/test/utils" | ||
. "github.com/onsi/ginkgo" | ||
|
@@ -499,4 +500,46 @@ var _ = Describe("Podman create", func() { | |
Expect(data[0].Config.Timezone).To(Equal("local")) | ||
}) | ||
|
||
It("podman create --umask", func() { | ||
if !strings.Contains(podmanTest.OCIRuntime, "crun") { | ||
Skip("Test only works on crun") | ||
} | ||
|
||
session := podmanTest.Podman([]string{"create", "--name", "default", ALPINE}) | ||
session.WaitWithDefaultTimeout() | ||
inspect := podmanTest.Podman([]string{"inspect", "default"}) | ||
inspect.WaitWithDefaultTimeout() | ||
data := inspect.InspectContainerToJSON() | ||
Expect(len(data)).To(Equal(1)) | ||
Expect(data[0].Config.Umask).To(Equal("0022")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coud you add a test for podman run --umask 0077 fedora umask |
||
|
||
session = podmanTest.Podman([]string{"create", "--umask", "0002", "--name", "umask", ALPINE}) | ||
session.WaitWithDefaultTimeout() | ||
inspect = podmanTest.Podman([]string{"inspect", "umask"}) | ||
inspect.WaitWithDefaultTimeout() | ||
data = inspect.InspectContainerToJSON() | ||
Expect(len(data)).To(Equal(1)) | ||
Expect(data[0].Config.Umask).To(Equal("0002")) | ||
|
||
session = podmanTest.Podman([]string{"create", "--umask", "0077", "--name", "fedora", fedoraMinimal}) | ||
session.WaitWithDefaultTimeout() | ||
inspect = podmanTest.Podman([]string{"inspect", "fedora"}) | ||
inspect.WaitWithDefaultTimeout() | ||
data = inspect.InspectContainerToJSON() | ||
Expect(len(data)).To(Equal(1)) | ||
Expect(data[0].Config.Umask).To(Equal("0077")) | ||
|
||
session = podmanTest.Podman([]string{"create", "--umask", "22", "--name", "umask-short", ALPINE}) | ||
session.WaitWithDefaultTimeout() | ||
inspect = podmanTest.Podman([]string{"inspect", "umask-short"}) | ||
inspect.WaitWithDefaultTimeout() | ||
data = inspect.InspectContainerToJSON() | ||
Expect(len(data)).To(Equal(1)) | ||
Expect(data[0].Config.Umask).To(Equal("0022")) | ||
|
||
session = podmanTest.Podman([]string{"create", "--umask", "9999", "--name", "bad", ALPINE}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Not(Equal(0))) | ||
Expect(session.ErrorToString()).To(ContainSubstring("Invalid umask")) | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1081,4 +1081,35 @@ USER mail` | |
Expect(session.ExitCode()).To(Equal(0)) | ||
Expect(session.OutputToString()).To(ContainSubstring(limit)) | ||
}) | ||
|
||
It("podman run umask", func() { | ||
if !strings.Contains(podmanTest.OCIRuntime, "crun") { | ||
Skip("Test only works on crun") | ||
} | ||
|
||
session := podmanTest.Podman([]string{"run", "--rm", ALPINE, "sh", "-c", "umask"}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Equal(0)) | ||
Expect(session.OutputToString()).To(Equal("0022")) | ||
|
||
session = podmanTest.Podman([]string{"run", "--umask", "0002", "--rm", ALPINE, "sh", "-c", "umask"}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Equal(0)) | ||
Expect(session.OutputToString()).To(Equal("0002")) | ||
|
||
session = podmanTest.Podman([]string{"run", "--umask", "0077", "--rm", fedoraMinimal, "umask"}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Equal(0)) | ||
Expect(session.OutputToString()).To(Equal("0077")) | ||
|
||
session = podmanTest.Podman([]string{"run", "--umask", "22", "--rm", ALPINE, "sh", "-c", "umask"}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Equal(0)) | ||
Expect(session.OutputToString()).To(Equal("0022")) | ||
|
||
session = podmanTest.Podman([]string{"run", "--umask", "9999", "--rm", ALPINE, "sh", "-c", "umask"}) | ||
session.WaitWithDefaultTimeout() | ||
Expect(session.ExitCode()).To(Not(Equal(0))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, will keep in mind for future tests too :) |
||
Expect(session.ErrorToString()).To(ContainSubstring("Invalid umask")) | ||
}) | ||
}) |
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 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.
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.
The value is actually an octal. Storing it as an int, may make it hard to convert and hard to view in inspect.
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.
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