fix containerized function mounts issue (kubernetes-sigs#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

Author: natasha41575

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)
-
-	th.WriteK(".", `
+	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(`
 resources:
 - data1.yaml
 - data2.yaml
 transformers:
 - label_namespace.yaml
-`)
-
-	th.WriteF("data1.yaml", `apiVersion: v1
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "data1.yaml"), []byte(`
+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
-`)
-
-	th.WriteF("label_namespace.yaml", `apiVersion: v1
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "label_namespace.yaml"), []byte(`
+apiVersion: v1
 kind: ConfigMap
 metadata:
   name: label_namespace
@@ -569,11 +573,14 @@ metadata:
 data:
   label_name: my-ns-name
   label_value: function-test
-`)
-
-	m := th.Run(".", th.MakeOptionsPluginsEnabled())
-	th.AssertActualEqualsExpected(m, `
-apiVersion: v1
+`)))
+	m, err := b.Run(
+		fSys,
+		tmpDir.String())
+	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)
-
-	th.WriteK(".", `
+	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
-`)
-
-	// 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", `
+`)))
+	assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "gener.yaml"), []byte(`
 apiVersion: v1
 kind: ConfigMap
 metadata:
@@ -622,14 +627,176 @@ data:
       name: env
     data:
       value: '{{ env "TESTTEMPLATE" }}'
-`)
-	m := th.Run(".", th.MakeOptionsPluginsEnabled())
-	th.AssertActualEqualsExpected(m, `
-apiVersion: v1
+`)))
+	m, err := b.Run(
+		fSys,
+		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 {
-		return err
+
+	if c.Exec.WorkingDir == "" {
+		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())
 	}