fix containerized function mounts issue (#4489)

* fix containerized function mounts issue

* skip path test on windows

* move test out of temp dir

* update tests to deal with new working dir restrictions

* code review

Makefile

@@ -82,7 +82,7 @@ kustomize: $(MYGOBIN)/kustomize
 # Used to add non-default compilation flags when experimenting with
 # plugin-to-api compatibility checks.
 .PHONY: build-kustomize-api
-build-kustomize-api: $(builtinplugins)
+build-kustomize-api: $(MYGOBIN)/goimports $(builtinplugins)
 	cd api; $(MAKE) build
 
 .PHONY: generate-kustomize-api

api/internal/plugins/loader/loader.go

@@ -228,6 +228,17 @@ func (l *Loader) makeBuiltinPlugin(r resid.Gvk) (resmap.Configurable, error) {
 func (l *Loader) loadPlugin(res *resource.Resource) (resmap.Configurable, error) {
 	spec := fnplugin.GetFunctionSpec(res)
 	if spec != nil {
+		// validation check that function mounts are under the current kustomization directory
+		for _, mount := range spec.Container.StorageMounts {
+			if filepath.IsAbs(mount.Src) {
+				return nil, errors.New(fmt.Sprintf("plugin %s with mount path '%s' is not permitted; "+
+					"mount paths must be relative to the current kustomization directory", res.OrgId(), mount.Src))
+			}
+			if strings.HasPrefix(filepath.Clean(mount.Src), "../") {
+				return nil, errors.New(fmt.Sprintf("plugin %s with mount path '%s' is not permitted; "+
+					"mount paths must be under the current kustomization directory", res.OrgId(), mount.Src))
+			}
+		}
 		return fnplugin.NewFnPlugin(&l.pc.FnpLoadingOptions), nil
 	}
 	return l.loadExecOrGoPlugin(res.OrgId())

api/krusty/fnplugin_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	. "sigs.k8s.io/kustomize/api/krusty"
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 	"sigs.k8s.io/kustomize/kyaml/filesys"
 )
@@ -535,30 +536,33 @@ spec:
 
 func TestFnContainerTransformerWithConfig(t *testing.T) {
 	skipIfNoDocker(t)
-
-	// Function plugins should not need the env setup done by MakeEnhancedHarness
 	th := kusttest_test.MakeHarness(t)
-
+	o := th.MakeOptionsPluginsEnabled()
-	th.WriteK(".", `
+	fSys := filesys.MakeFsOnDisk()
+	b := MakeKustomizer(&o)
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
 resources:
 - data1.yaml
 - data2.yaml
 transformers:
 - label_namespace.yaml
-`)
+`)))
-
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "data1.yaml"), []byte(`
-	th.WriteF("data1.yaml", `apiVersion: v1
+apiVersion: v1
 kind: Namespace
 metadata:
   name: my-namespace
-`)
+`)))
-	th.WriteF("data2.yaml", `apiVersion: v1
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "data2.yaml"), []byte(`
+apiVersion: v1
 kind: Namespace
 metadata:
   name: another-namespace
-`)
+`)))
-
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "label_namespace.yaml"), []byte(`
-	th.WriteF("label_namespace.yaml", `apiVersion: v1
+apiVersion: v1
 kind: ConfigMap
 metadata:
   name: label_namespace
@@ -569,11 +573,14 @@ metadata:
 data:
   label_name: my-ns-name
   label_value: function-test
-`)
+`)))
-
+	m, err := b.Run(
-	m := th.Run(".", th.MakeOptionsPluginsEnabled())
+		fSys,
-	th.AssertActualEqualsExpected(m, `
+		tmpDir.String())
-apiVersion: v1
+	assert.NoError(t, err)
+	actual, err := m.AsYaml()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: v1
 kind: Namespace
 metadata:
   labels:
@@ -586,24 +593,22 @@ metadata:
   labels:
     my-ns-name: function-test
   name: another-namespace
-`)
+`, string(actual))
 }
 
 func TestFnContainerEnvVars(t *testing.T) {
 	skipIfNoDocker(t)
-
-	// Function plugins should not need the env setup done by MakeEnhancedHarness
 	th := kusttest_test.MakeHarness(t)
-
+	o := th.MakeOptionsPluginsEnabled()
-	th.WriteK(".", `
+	fSys := filesys.MakeFsOnDisk()
+	b := MakeKustomizer(&o)
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
 generators:
 - gener.yaml
-`)
+`)))
-
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "gener.yaml"), []byte(`
-	// TODO: cheange image to gcr.io/kpt-functions/templater:stable
-	// when https://github.com/GoogleContainerTools/kpt-functions-catalog/pull/103
-	// is merged
-	th.WriteF("gener.yaml", `
 apiVersion: v1
 kind: ConfigMap
 metadata:
@@ -622,14 +627,176 @@ data:
       name: env
     data:
       value: '{{ env "TESTTEMPLATE" }}'
-`)
+`)))
-	m := th.Run(".", th.MakeOptionsPluginsEnabled())
+	m, err := b.Run(
-	th.AssertActualEqualsExpected(m, `
+		fSys,
-apiVersion: v1
+		tmpDir.String())
+	assert.NoError(t, err)
+	actual, err := m.AsYaml()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: v1
 data:
   value: value
 kind: ConfigMap
 metadata:
   name: env
-`)
+`, string(actual))
+}
+
+func TestFnContainerFnMounts(t *testing.T) {
+	skipIfNoDocker(t)
+	th := kusttest_test.MakeHarness(t)
+	o := th.MakeOptionsPluginsEnabled()
+	fSys := filesys.MakeFsOnDisk()
+	b := MakeKustomizer(&o)
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
+generators:
+- gener.yaml
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "gener.yaml"), []byte(`
+apiVersion: v1alpha1
+kind: RenderHelmChart
+metadata:
+  name: demo
+  annotations:
+    config.kubernetes.io/function: |
+      container:
+        image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
+        mounts:
+        - type: "bind"
+          src: "./charts"
+          dst: "/tmp/charts"
+helmCharts:
+- name: helloworld-chart
+  releaseName: test
+  valuesFile: /tmp/charts/helloworld-values/values.yaml
+`)))
+	assert.NoError(t, fSys.MkdirAll(filepath.Join(tmpDir.String(), "charts", "helloworld-chart", "templates")))
+	assert.NoError(t, fSys.MkdirAll(filepath.Join(tmpDir.String(), "charts", "helloworld-values")))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "charts", "helloworld-chart", "Chart.yaml"), []byte(`
+apiVersion: v2
+name: helloworld-chart
+description: A Helm chart for Kubernetes
+type: application
+version: 0.1.0
+appVersion: 1.16.0
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "charts", "helloworld-chart", "templates", "deployment.yaml"), []byte(`
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: name
+spec:
+  replicas: {{ .Values.replicaCount }}
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "charts", "helloworld-values", "values.yaml"), []byte(`
+replicaCount: 5
+`)))
+	m, err := b.Run(
+		fSys,
+		tmpDir.String())
+	assert.NoError(t, err)
+	actual, err := m.AsYaml()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: name
+spec:
+  replicas: 5
+`, string(actual))
+}
+
+func TestFnContainerMountsLoadRestrictions_absolute(t *testing.T) {
+	skipIfNoDocker(t)
+	th := kusttest_test.MakeHarness(t)
+	o := th.MakeOptionsPluginsEnabled()
+	fSys := filesys.MakeFsOnDisk()
+	b := MakeKustomizer(&o)
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
+generators:
+  - |-
+    apiVersion: v1alpha1
+    kind: RenderHelmChart
+    metadata:
+      name: demo
+      annotations:
+        config.kubernetes.io/function: |
+          container:
+            image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
+            mounts:
+            - type: "bind"
+              src: "/tmp/dir"
+              dst: "/tmp/charts"
+`)))
+	_, err = b.Run(
+		fSys,
+		tmpDir.String())
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "loading generator plugins: plugin RenderHelmChart."+
+		"v1alpha1.[noGrp]/demo.[noNs] with mount path '/tmp/dir' is not permitted; mount paths must"+
+		" be relative to the current kustomization directory")
+}
+
+func TestFnContainerMountsLoadRestrictions_outsideCurrentDir(t *testing.T) {
+	skipIfNoDocker(t)
+	th := kusttest_test.MakeHarness(t)
+	o := th.MakeOptionsPluginsEnabled()
+	fSys := filesys.MakeFsOnDisk()
+	b := MakeKustomizer(&o)
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
+generators:
+  - |-
+    apiVersion: v1alpha1
+    kind: RenderHelmChart
+    metadata:
+      name: demo
+      annotations:
+        config.kubernetes.io/function: |
+          container:
+            image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
+            mounts:
+            - type: "bind"
+              src: "./tmp/../../dir"
+              dst: "/tmp/charts"
+`)))
+	_, err = b.Run(
+		fSys,
+		tmpDir.String())
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "loading generator plugins: plugin RenderHelmChart."+
+		"v1alpha1.[noGrp]/demo.[noNs] with mount path './tmp/../../dir' is not permitted; mount paths must "+
+		"be under the current kustomization directory")
+}
+
+func TestFnContainerMountsLoadRestrictions_root(t *testing.T) {
+	skipIfNoDocker(t)
+	th := kusttest_test.MakeHarness(t)
+
+	th.WriteK(".", `
+generators:
+- gener.yaml
+`)
+	// Create generator config
+	th.WriteF("gener.yaml", `
+apiVersion: examples.config.kubernetes.io/v1beta1
+kind: CockroachDB
+metadata:
+  name: demo
+  annotations:
+    config.kubernetes.io/function: |
+      container:
+        image: gcr.io/kustomize-functions/example-cockroachdb:v0.1.0
+spec:
+  replicas: 3
+`)
+	err := th.RunWithErr(".", th.MakeOptionsPluginsEnabled())
+	assert.Error(t, err)
+	assert.EqualError(t, err, "couldn't execute function: root working directory '/' not allowed")
 }

kyaml/fn/runtime/container/container.go

@@ -6,10 +6,11 @@ package container
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 
+	"sigs.k8s.io/kustomize/kyaml/errors"
 	runtimeexec "sigs.k8s.io/kustomize/kyaml/fn/runtime/exec"
 	"sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil"
-
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
@@ -151,11 +152,14 @@ func (c *Filter) setupExec() error {
 	if c.Exec.Path != "" {
 		return nil
 	}
-	wd, err := os.Getwd()
+
-	if err != nil {
+	if c.Exec.WorkingDir == "" {
-		return err
+		wd, err := os.Getwd()
+		if err != nil {
+			return errors.Wrap(err)
+		}
+		c.Exec.WorkingDir = wd
 	}
-	c.Exec.WorkingDir = wd
 
 	path, args := c.getCommand()
 	c.Exec.Path = path
@@ -183,8 +187,11 @@ func (c *Filter) getCommand() (string, []string) {
 		// note: don't make fs readonly because things like heredoc rely on writing tmp files
 	}
 
-	// TODO(joncwong): Allow StorageMount fields to have default values.
 	for _, storageMount := range c.StorageMounts {
+		// convert declarative relative paths to absolute (otherwise docker will throw an error)
+		if !filepath.IsAbs(storageMount.Src) {
+			storageMount.Src = filepath.Join(c.Exec.WorkingDir, storageMount.Src)
+		}
 		args = append(args, "--mount", storageMount.String())
 	}
 

kyaml/fn/runtime/container/container_test.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -81,18 +82,19 @@ metadata:
 				"--network", "none",
 				"--user", "nobody",
 				"--security-opt=no-new-privileges",
-				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "bind", "/mount/path", "/local/"),
+				// use filepath.Join for Windows filepath handling
-				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s", "bind", "/mount/pathrw", "/localrw/"),
+				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "bind", getAbsFilePath(string(filepath.Separator), "mount", "path"), "/local/"),
-				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "volume", "myvol", "/local/"),
+				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s", "bind", getAbsFilePath(string(filepath.Separator), "mount", "pathrw"), "/localrw/"),
-				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "tmpfs", "", "/local/"),
+				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "volume", getAbsFilePath(string(filepath.Separator), "myvol"), "/local/"),
+				"--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "tmpfs", getAbsFilePath(string(filepath.Separator)), "/local/"),
 			},
 			containerSpec: runtimeutil.ContainerSpec{
 				Image: "example.com:version",
 				StorageMounts: []runtimeutil.StorageMount{
-					{MountType: "bind", Src: "/mount/path", DstPath: "/local/"},
+					{MountType: "bind", Src: getAbsFilePath(string(filepath.Separator), "mount", "path"), DstPath: "/local/"},
-					{MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true},
+					{MountType: "bind", Src: getAbsFilePath(string(filepath.Separator), "mount", "pathrw"), DstPath: "/localrw/", ReadWriteMode: true},
-					{MountType: "volume", Src: "myvol", DstPath: "/local/"},
+					{MountType: "volume", Src: getAbsFilePath(string(filepath.Separator), "myvol"), DstPath: "/local/"},
-					{MountType: "tmpfs", Src: "", DstPath: "/local/"},
+					{MountType: "tmpfs", Src: getAbsFilePath(string(filepath.Separator)), DstPath: "/local/"},
 				},
 			},
 			UIDGID: "nobody",
@@ -247,3 +249,8 @@ func getWorkingDir(t *testing.T) string {
 	require.NoError(t, err)
 	return wd
 }
+
+func getAbsFilePath(args ...string) string {
+	path, _ := filepath.Abs(filepath.Join(args...))
+	return path
+}

kyaml/fn/runtime/runtimeutil/functiontypes.go

@@ -136,9 +136,6 @@ type FunctionSpec struct {
 
 	// ExecSpec is the spec for running a function as an executable
 	Exec ExecSpec `json:"exec,omitempty" yaml:"exec,omitempty"`
-
-	// Mounts are the storage or directories to mount into the container
-	StorageMounts []StorageMount `json:"mounts,omitempty" yaml:"mounts,omitempty"`
 }
 
 type ExecSpec struct {
@@ -208,9 +205,7 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec {
 	if err != nil {
 		return nil
 	}
-
 	if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil {
-		fn.StorageMounts = []StorageMount{}
 		return fn
 	}
 

kyaml/runfn/runfn.go

@@ -329,8 +329,11 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) (
 			continue
 		}
 		cf, ok := c.(*container.Filter)
-		if global && ok {
+		if ok {
-			cf.Exec.GlobalScope = true
+			if global {
+				cf.Exec.GlobalScope = true
+			}
+			cf.Exec.WorkingDir = r.WorkingDir
 		}
 		fltrs = append(fltrs, c)
 	}
@@ -468,11 +471,17 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode, currentUser
 		if err != nil {
 			return nil, err
 		}
+
+		// Storage mounts can either come from kustomize fn run --mounts,
+		// or from the declarative function mounts field.
+		storageMounts := spec.Container.StorageMounts
+		storageMounts = append(storageMounts, r.StorageMounts...)
+
 		c := container.NewContainer(
 			runtimeutil.ContainerSpec{
 				Image:         spec.Container.Image,
 				Network:       spec.Container.Network,
-				StorageMounts: r.StorageMounts,
+				StorageMounts: storageMounts,
 				Env:           spec.Container.Env,
 			},
 			uidgid,

plugin/builtin/hashtransformer/HashTransformer_test.go

@@ -6,7 +6,7 @@ package main_test
 import (
 	"testing"
 
-	"sigs.k8s.io/kustomize/api/testutils/kusttest"
+	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
 func TestHashTransformer(t *testing.T) {