common/overlay: allow data directory name with colon character #3505

ybubnov · 2017-01-03T13:22:31Z

This patch provides an sanitizing function used to escape the colon characters in the directory names of the mount command.

According to the Linux documentation https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt it
is used as a separator symbol in case of multi lower-layer mount.

It also provides the unit tests for common/overlay package, that is possible to execute only with superuser privileges.

Fixes #3448

ghost · 2017-01-03T13:22:33Z

Can one of the admins verify this patch?

jonboulle · 2017-01-03T13:38:55Z

@rktbot ok to test

ybubnov · 2017-01-03T13:48:31Z

common/overlay/overlay_test.go

+}
+
+func TestMount(t *testing.T) {
+ if os.Geteuid() != 0 {


Unfortunately, Go syscall wrapper does not allow (due to mentioned in the comments reason) to gain root privileges: syscall/syscall_linux.go#L861. Link to the original issue: golang/go#1435.

Therefore I didn't find anything smarter than run these tests only when they've been launched with superuser privileges.

jonboulle · 2017-01-03T14:07:31Z

I'm pretty leery of testing actual mounting/overlay behaviour here. What about just splitting out the construction of the mount incantation into a function and testing that instead?

ybubnov · 2017-01-03T14:11:05Z

I'm pretty leery of testing actual mounting/overlay behaviour here.

When you say leery, you want me to remove the introduced tests?

What about just splitting out the construction of the mount incantation into a function and testing that instead?

Sure, I will do it.

jonboulle · 2017-01-03T14:15:55Z

When you say leery, you want me to remove the introduced tests?

Sorry, that was a bit vague - I mean I'm not sure it's a good idea for a unit test since it has particular requirements (an overlay-supporting kernel, root permissions) and can be invasive to the host OS (messing with mounts). But come to think of it, maybe it's something we could add to the functional tests? It is nice to have such comprehensive behaviour testing, and the functional tests already have such demands - https:/coreos/rkt/tree/master/tests

ybubnov · 2017-01-03T14:19:42Z

Oh I see, thank you. This sounds reasonable to me, I will move that tests into the tests sub-directory.

ybubnov · 2017-01-03T15:37:21Z

That is strange, semaphoreci failed on ./tests/install-deps.sh # Setup step: https://semaphoreci.com/coreos/rkt/branches/pull-request-3505/builds/2

I will resubmit the latest changes to trigger the build again.

jonboulle · 2017-01-03T15:46:43Z

That looks like a Semaphore flake, probably they're upgrading their OS or something

…

On 3 January 2017 at 16:37, Yasha Bubnov ***@***.***> wrote: That is strange, semaphoreci failed on ./tests/install-deps.sh # Setup step: https://semaphoreci.com/coreos/rkt/branches/pull- request-3505/builds/2 I will resubmit the latest changes to trigger the build again. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3505 (comment)>, or mute the thread <https:/notifications/unsubscribe-auth/ACewNwTMfQSuijb9jujvlwZcTRlX6dnuks5rOms1gaJpZM4LZnPL> .

ybubnov · 2017-01-03T17:09:02Z

Looks the kernel of machines provided by semaphoreci does not support overlayfs (at least one of the existing tests reported this) https://semaphoreci.com/coreos/rkt/branches/pull-request-3505/builds/4:

=== RUN   TestGCAfterUnmount
--- SKIP: TestGCAfterUnmount (0.00s)
	rkt_gc_test.go:84: Overlay fs not supported: overlay entry not present in /proc/filesystems

I added the same check to rkt_mount_overlay_test.go.

s-urbaniak

Just a few nits and a small question.

s-urbaniak · 2017-01-05T15:40:24Z

common/overlay/overlay.go

+// The opts returns options for mount system call.
+func (cfg *MountCfg) opts() string {
+ opts := fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s",
+ sanitize(cfg.Lower), sanitize(cfg.Upper), sanitize(cfg.Work))


Just a nit to be consistent:

fmt.Sprintf( "lowerdir=%s,upperdir=%s,workdir=%s", sanitize(cfg.Lower), sanitize(cfg.Upper), sanitize(cfg.Work), ),

s-urbaniak · 2017-01-05T15:48:04Z

common/overlay/overlay.go

+ return strings.Replace(dir, ":", "\\:", -1)
+}
+
+// The opts returns options for mount system call.


// opts returns options for mount system call.

s-urbaniak · 2017-01-05T15:49:01Z

tests/rkt_tests.go

@@ -219,6 +219,19 @@ func createTempDirOrPanic(dirName string) string {
 return tmpDir
 }

+// The createFileOrPanic creates an empty file within the given directory
+// with a specified name. Panics if file creation fails for any reason.
+func createFileOrPanic(dirName, fileName string) string {


// createFileOrPanic ...

s-urbaniak · 2017-01-05T15:49:48Z

common/overlay/overlay.go

+// The sanitize escapes the colon symbol in order to support the dir names
+// with this character, otherwise it will be treated as a separator between
+// the directory names.
+func sanitize(dir string) string {


// sanitize escapes the ...

s-urbaniak · 2017-01-05T15:54:56Z

common/overlay/overlay.go

+}
+
+// The opts returns options for mount system call.
+func (cfg *MountCfg) opts() string {


This could be public, i.e. Opts(), since it has no side-effects.

s-urbaniak · 2017-01-05T15:59:45Z

tests/rkt_mount_overlay_test.go

+)
+
+func overlayMount(cfg overlay.MountCfg) error {
+ // Create a temporary directories with a configured prefix.


// Create temporary directories...

s-urbaniak · 2017-01-05T16:04:46Z

tests/rkt_tests.go

+// with a specified name. Panics if file creation fails for any reason.
+func createFileOrPanic(dirName, fileName string) string {
+ name := filepath.Join(dirName, fileName)
+ file, err := os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)


why not simply os.Create(...)? The only difference would be that it is created using 0666 mode vs 0600, but I don't think that matters here.

Right, it does not matter in this situation, I initially thought about close-exec flag, but as this function used only in tests this is, probably, not that important as well. So as you proposed, I changed to os.Create function.

s-urbaniak · 2017-01-06T08:39:55Z

@ybubnov looking good, thanks a lot! Do you mind squashing to one commit? From my side LGTM, handing over to @lucab for final review.

ybubnov · 2017-01-06T08:57:08Z

Squashed the changes into the single commit.

lucab · 2017-01-10T09:43:49Z

Thanks for the patch @ybubnov! I may be wrong, but I think , should be escaped as well. To double-check, can you test a path with an embedded comma and see if it causes a failure?

ybubnov · 2017-01-10T10:15:53Z

@lucab, I tested the mount command with coma in the directory names. Without escaping it does not work as well, therefore I slightly modified the patch and added required tests.

lucab · 2017-01-10T14:52:46Z

common/overlay/overlay.go

@@ -23,6 +23,10 @@ import (
 "github.com/hashicorp/errwrap"
 )

+// sanitizer defines a string translator used to escape colon and coma


Minor grammar issue here and in other place in this commit: "comma" (with double m).

lucab · 2017-01-10T14:58:23Z

common/overlay/overlay.go

 "syscall"

 "github.com/coreos/rkt/pkg/label"
 "github.com/hashicorp/errwrap"
 )

+// sanitizer defines a string translator used to escape colon and coma
+// characters in the directories names.
+var sanitizer = strings.NewReplacer(":", "\\:", ",", "\\,")


Sorry for not spotting this earlier, but if I understand correctly the \\ (double-slash) in here is just for golang string escaping, right? To make a bit clearer that we are doing the escaping for overlayfs here, would you mind changing this into raw strings with a single slash?

lucab · 2017-01-10T14:59:40Z

common/overlay/overlay_test.go

+ {MountCfg{"/tmp/test,1", "/tmp/test,2", "/tmp/test,3", "", ""},
+ "lowerdir=/tmp/test\\,1,upperdir=/tmp/test\\,2,workdir=/tmp/test\\,3"},
+ {MountCfg{"/tmp/,1,1", "/tmp/,,2", "/tmp/,3,", "", ""},
+ "lowerdir=/tmp/\\,1\\,1,upperdir=/tmp/\\,\\,2,workdir=/tmp/\\,3\\,"},


Same for the double-slash, escaping and raw strings here.

ybubnov · 2017-01-10T15:40:39Z

common/overlay/overlay.go

 // characters in the directories names.
-var sanitizer = strings.NewReplacer(":", "\\:", ",", "\\,")
+var sanitizer = strings.NewReplacer(`:`, `\:`, `,`, `\,`)


I redefined all characters as raw strings for the sake of consistency.

lucab · 2017-01-10T16:43:33Z

I'm fine with this now, thanks. Just squash the changes together in the first single commit and we should be ready to go!

ybubnov · 2017-01-10T16:47:27Z

Thanks, squashed the changes into the single commit.

This patch provides an sanitizing function used to escape the colon characters in the directory names of the mount command. According to the Linux documentation https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt it is used as a separator symbol in case of multi lower-layer mount. Separates the mount options rendering, thus make it possible to write pure unit tests for common/overlay package. Functional tests, that validates common/overlay.Mount function moved to the "tests" sub-directory.

ybubnov · 2017-01-11T09:22:27Z

It seems, introduced changes are constantly causing the failure of the TestAppUserGroup:

23:30:39 --- FAIL: TestAppUserGroup (119.08s)
...
23:30:39 		Sending SIGTERM to remaining processes...
23:30:39 		Sending SIGKILL to remaining processes...
23:30:39 		Container rkt-6ad75081-ecc6-499a-8db7-33851d34d370 failed with error code 226.
23:30:39 	rkt_tests.go:141: rkt terminated with unexpected status 226, expected 0
23:30:39 		Output:
23:30:39 		[]

I don't know if it is a known issue: #3477, or caused by new code.

lucab · 2017-01-11T09:58:00Z

No worry, that is a known ephemeral test failure we are currently chasing, unrelated to your PR.

lucab

LGTM

jonboulle · 2017-01-11T10:11:36Z

@ybubnov thanks so much for your work here!

ybubnov commented Jan 3, 2017

View reviewed changes

lucab added this to the v1.23.0 milestone Jan 5, 2017

s-urbaniak suggested changes Jan 5, 2017

View reviewed changes

s-urbaniak approved these changes Jan 6, 2017

View reviewed changes

s-urbaniak requested a review from lucab January 6, 2017 15:22

lucab added kind/bug needs/more-information technology/linux labels Jan 10, 2017

lucab reviewed Jan 10, 2017

View reviewed changes

ybubnov commented Jan 10, 2017

View reviewed changes

lucab approved these changes Jan 11, 2017

View reviewed changes

lucab added reviewed/lgtm reviewed/merge-on-green and removed needs/more-information labels Jan 11, 2017

lucab merged commit 4de6e93 into rkt:master Jan 11, 2017

ybubnov deleted the escape-colon-in-overlay-directory-names branch January 11, 2017 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common/overlay: allow data directory name with colon character #3505

common/overlay: allow data directory name with colon character #3505

ybubnov commented Jan 3, 2017

ghost commented Jan 3, 2017

jonboulle commented Jan 3, 2017

ybubnov Jan 3, 2017

jonboulle commented Jan 3, 2017

ybubnov commented Jan 3, 2017

jonboulle commented Jan 3, 2017 •

edited

Loading

ybubnov commented Jan 3, 2017

ybubnov commented Jan 3, 2017

jonboulle commented Jan 3, 2017 via email

ybubnov commented Jan 3, 2017

s-urbaniak left a comment

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

s-urbaniak Jan 5, 2017

ybubnov Jan 5, 2017 •

edited

Loading

s-urbaniak commented Jan 6, 2017

ybubnov commented Jan 6, 2017

lucab commented Jan 10, 2017 •

edited

Loading

ybubnov commented Jan 10, 2017 •

edited

Loading

lucab Jan 10, 2017 •

edited

Loading

lucab Jan 10, 2017 •

edited

Loading

lucab Jan 10, 2017

ybubnov Jan 10, 2017

lucab Jan 10, 2017

lucab commented Jan 10, 2017

ybubnov commented Jan 10, 2017

ybubnov commented Jan 11, 2017

lucab commented Jan 11, 2017

lucab left a comment

jonboulle commented Jan 11, 2017

common/overlay: allow data directory name with colon character #3505

common/overlay: allow data directory name with colon character #3505

Conversation

ybubnov commented Jan 3, 2017

ghost commented Jan 3, 2017

jonboulle commented Jan 3, 2017

Choose a reason for hiding this comment

jonboulle commented Jan 3, 2017

ybubnov commented Jan 3, 2017

jonboulle commented Jan 3, 2017 • edited Loading

ybubnov commented Jan 3, 2017

ybubnov commented Jan 3, 2017

jonboulle commented Jan 3, 2017 via email

ybubnov commented Jan 3, 2017

s-urbaniak left a comment

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

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

ybubnov Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

s-urbaniak commented Jan 6, 2017

ybubnov commented Jan 6, 2017

lucab commented Jan 10, 2017 • edited Loading

ybubnov commented Jan 10, 2017 • edited Loading

lucab Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

lucab Jan 10, 2017 • edited Loading

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

lucab commented Jan 10, 2017

ybubnov commented Jan 10, 2017

ybubnov commented Jan 11, 2017

lucab commented Jan 11, 2017

lucab left a comment

Choose a reason for hiding this comment

jonboulle commented Jan 11, 2017

jonboulle commented Jan 3, 2017 •

edited

Loading

ybubnov Jan 5, 2017 •

edited

Loading

lucab commented Jan 10, 2017 •

edited

Loading

ybubnov commented Jan 10, 2017 •

edited

Loading

lucab Jan 10, 2017 •

edited

Loading

lucab Jan 10, 2017 •

edited

Loading