From 63f9f79fc07db26a6903da26fa4862b786203e9a Mon Sep 17 00:00:00 2001 From: Alexey Odinokov Date: Sat, 11 Jul 2020 05:21:42 +0000 Subject: [PATCH 1/2] Fixed incorrect docker mount arguments generation The previous implementation combined --mount and -v notation of argument [1] adding :ro to make the read-only mount point. E.g. the command [2] called docker with the following params: [3]. As a result instead of the read-only folder /tmp/source, the read-write folder /tmp/source/:ro/' is created. This PR: 1. substitutes ':ro' with correct ',readonly'. 2. changes 'src=' and 'dst=' with 'source=' and 'target=' as it is stated in the documentation [1] 3. introduces the ability to EXPLICITLY create a mountpoint with read-write access. To do so it's necessary to add ',rw=true' to the --mount argument 4. corrects UTs adds some additional coverage for added functionality [1] https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount [2] kustomize fn run ./d --mount type=bind,src=$(pwd)/test/,dst=/tmp/source/ [3] --mount type=bind,src=/home/ubuntu/kpt-functions-catalog/functions/ts/test/,dst=/tmp/source/:ro --- kyaml/fn/runtime/container/container_test.go | 8 ++-- kyaml/fn/runtime/runtimeutil/functiontypes.go | 16 +++++++- .../runtime/runtimeutil/runtimeutil_test.go | 41 +++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index 17bf65095..2e054f945 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -73,14 +73,16 @@ metadata: "--network", "none", "--user", "nobody", "--security-opt=no-new-privileges", - "--mount", fmt.Sprintf("type=%s,src=%s,dst=%s:ro", "bind", "/mount/path", "/local/"), - "--mount", fmt.Sprintf("type=%s,src=%s,dst=%s:ro", "volume", "myvol", "/local/"), - "--mount", fmt.Sprintf("type=%s,src=%s,dst=%s:ro", "tmpfs", "", "/local/"), + "--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "bind", "/mount/path", "/local/"), + "--mount", fmt.Sprintf("type=%s,source=%s,target=%s", "bind", "/mount/pathrw", "/localrw/"), + "--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "volume", "myvol", "/local/"), + "--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "tmpfs", "", "/local/"), }, instance: Filter{ Image: "example.com:version", StorageMounts: []runtimeutil.StorageMount{ {MountType: "bind", Src: "/mount/path", DstPath: "/local/"}, + {MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true}, {MountType: "volume", Src: "myvol", DstPath: "/local/"}, {MountType: "tmpfs", Src: "", DstPath: "/local/"}, }, diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 031857342..9bd396d30 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -84,10 +84,18 @@ type StorageMount struct { // The path where the file or directory is mounted in the container. DstPath string `json:"dst,omitempty" yaml:"dst,omitempty"` + + // Mount in ReadWrite mode if it's explicitly configured + // See https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount + ReadWriteMode bool `json:"rw,omitempty" yaml:"rw,omitempty"` } func (s *StorageMount) String() string { - return fmt.Sprintf("type=%s,src=%s,dst=%s:ro", s.MountType, s.Src, s.DstPath) + mode := "" + if !s.ReadWriteMode { + mode = ",readonly" + } + return fmt.Sprintf("type=%s,source=%s,target=%s%s", s.MountType, s.Src, s.DstPath, mode) } // GetFunctionSpec returns the FunctionSpec for a resource. Returns @@ -149,7 +157,9 @@ func StringToStorageMount(s string) StorageMount { options := strings.Split(s, ",") for _, option := range options { keyVal := strings.SplitN(option, "=", 2) - m[keyVal[0]] = keyVal[1] + if len(keyVal) == 2 { + m[keyVal[0]] = keyVal[1] + } } var sm StorageMount for key, value := range m { @@ -160,6 +170,8 @@ func StringToStorageMount(s string) StorageMount { sm.Src = value case key == "dst": sm.DstPath = value + case key == "rw" && value == "true": + sm.ReadWriteMode = true } } return sm diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go index 60e026fcb..cbffcf042 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go @@ -1379,3 +1379,44 @@ metadata: assert.Equal(t, tc.required, fn.Container.Network.Required) } } + +func Test_StringToStorageMount(t *testing.T) { + tests := []struct { + in string + expectedOut string + }{ + { + in: "type=bind,src=/tmp/test/,dst=/tmp/source/", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/,readonly", + }, + { + in: "type=bind,src=/tmp/test/,dst=/tmp/source/,rw=true", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/", + }, + { + in: "type=bind,src=/tmp/test/,dst=/tmp/source/,rw=false", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/,readonly", + }, + { + in: "type=bind,src=/tmp/test/,dst=/tmp/source/,rw=", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/,readonly", + }, + { + in: "type=tmpfs,src=/tmp/test/,dst=/tmp/source/,rw=invalid", + expectedOut: "type=tmpfs,source=/tmp/test/,target=/tmp/source/,readonly", + }, + { + in: "type=tmpfs,src=/tmp/test/,dst=/tmp/source/,rwe=invalid", + expectedOut: "type=tmpfs,source=/tmp/test/,target=/tmp/source/,readonly", + }, + { + in: "type=tmpfs,src=/tmp/test/,dst", + expectedOut: "type=tmpfs,source=/tmp/test/,target=,readonly", + }, + } + + for _, tc := range tests { + s := StringToStorageMount(tc.in) + assert.Equal(t, tc.expectedOut, (&s).String()) + } +} From ba3e09849a8faddf93306f15ec58f5638c77b520 Mon Sep 17 00:00:00 2001 From: Alexey Odinokov Date: Mon, 13 Jul 2020 17:21:37 +0000 Subject: [PATCH 2/2] Made mountString params more similar to docker params see [1] kept support for the previous field names similarly to docker behavior. [1] https://docs.docker.com/storage/bind-mounts/#use-a-read-only-bind-mount --- kyaml/fn/runtime/runtimeutil/functiontypes.go | 4 ++-- kyaml/fn/runtime/runtimeutil/runtimeutil_test.go | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 9bd396d30..6e476e871 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -166,9 +166,9 @@ func StringToStorageMount(s string) StorageMount { switch { case key == "type": sm.MountType = value - case key == "src": + case key == "src" || key == "source": sm.Src = value - case key == "dst": + case key == "dst" || key == "target": sm.DstPath = value case key == "rw" && value == "true": sm.ReadWriteMode = true diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go index cbffcf042..89b728cf1 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go @@ -1413,6 +1413,14 @@ func Test_StringToStorageMount(t *testing.T) { in: "type=tmpfs,src=/tmp/test/,dst", expectedOut: "type=tmpfs,source=/tmp/test/,target=,readonly", }, + { + in: "type=bind,source=/tmp/test/,target=/tmp/source/,rw=true", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/", + }, + { + in: "type=bind,source=/tmp/test/,target=/tmp/source/", + expectedOut: "type=bind,source=/tmp/test/,target=/tmp/source/,readonly", + }, } for _, tc := range tests {