From 246c54a18b0cd4a6910fb75b18916c8c5244c757 Mon Sep 17 00:00:00 2001
From: Natasha Sarkar <natashasarkar@google.com>
Date: Wed, 18 Aug 2021 13:47:43 -0700
Subject: [PATCH] suggested changes

---
 api/internal/target/kusttarget.go      |  11 ++-
 api/krusty/fnplugin_test.go            | 118 +++++++++++++------------
 api/krusty/fnplugin_test/fnexectest.sh |  24 -----
 api/resmap/resmap.go                   |   4 -
 api/resmap/reswrangler.go              |  13 ---
 kyaml/fn/runtime/exec/exec.go          |  12 +--
 kyaml/kio/kioutil/kioutil.go           |   4 +
 kyaml/runfn/runfn.go                   |  28 ++++--
 8 files changed, 100 insertions(+), 114 deletions(-)
 delete mode 100755 api/krusty/fnplugin_test/fnexectest.sh

diff --git a/api/internal/target/kusttarget.go b/api/internal/target/kusttarget.go
index 046ed27a17..f601b42530 100644
--- a/api/internal/target/kusttarget.go
+++ b/api/internal/target/kusttarget.go
@@ -264,7 +264,7 @@ func (kt *KustTarget) configureExternalGenerators() ([]resmap.Generator, error)
 		return nil, err
 	}
 	m := ra.ResMap()
-	err = m.AnnotateAll(kioutil.PathAnnotation, kt.ldr.Root())
+	err = m.AnnotateAll(kioutil.WorkingDirAnnotation, kt.ldr.Root())
 	if err != nil {
 		return nil, err
 	}
@@ -300,12 +300,17 @@ func (kt *KustTarget) configureExternalTransformers(transformers []string) ([]re
 		}
 		ra.AppendAll(rm)
 	}
-	ra, err := kt.accumulateResources(ra, transformerPaths, &resource.Origin{})
 
+	ra, err := kt.accumulateResources(ra, transformerPaths, &resource.Origin{})
+	if err != nil {
+		return nil, err
+	}
+	m := ra.ResMap()
+	err = m.AnnotateAll(kioutil.WorkingDirAnnotation, kt.ldr.Root())
 	if err != nil {
 		return nil, err
 	}
-	return kt.pLdr.LoadTransformers(kt.ldr, kt.validator, ra.ResMap())
+	return kt.pLdr.LoadTransformers(kt.ldr, kt.validator, m)
 }
 
 func (kt *KustTarget) runValidators(ra *accumulator.ResAccumulator) error {
diff --git a/api/krusty/fnplugin_test.go b/api/krusty/fnplugin_test.go
index 554b5637f9..d4d7057ccf 100644
--- a/api/krusty/fnplugin_test.go
+++ b/api/krusty/fnplugin_test.go
@@ -7,17 +7,46 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"sigs.k8s.io/kustomize/api/internal/utils"
-	"sigs.k8s.io/kustomize/api/krusty"
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 	"sigs.k8s.io/kustomize/kyaml/filesys"
 )
 
+const execFile = `#!/bin/sh
+
+cat <<EOF
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx
+  labels:
+    app: nginx
+  annotations:
+    tshirt-size: small # this injects the resource reservations
+spec:
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+EOF
+`
+
 func TestFnExecGenerator(t *testing.T) {
-	// Function plugins should not need the env setup done by MakeEnhancedHarness
-	th := kusttest_test.MakeHarness(t)
+	fSys := filesys.MakeFsOnDisk()
 
-	th.WriteK(".", `
+	th := kusttest_test.MakeHarnessWithFs(t, fSys)
+	o := th.MakeOptionsPluginsEnabled()
+	o.PluginConfig.FnpLoadingOptions.EnableExec = true
+
+	tmpDir, err := filesys.NewTmpConfirmedDir()
+	assert.NoError(t, err)
+	th.WriteK(tmpDir.String(), `
 resources:
 - short_secret.yaml
 generators:
@@ -25,7 +54,8 @@ generators:
 `)
 
 	// Create some additional resource just to make sure everything is added
-	th.WriteF("short_secret.yaml", `
+	th.WriteF(filepath.Join(tmpDir.String(), "short_secret.yaml"),
+		`
 apiVersion: v1
 kind: Secret
 metadata:
@@ -38,22 +68,25 @@ stringData:
     bootcmd:
     - mkdir /mnt/vda
 `)
+	th.WriteF(filepath.Join(tmpDir.String(), "exec.sh"), execFile)
 
-	th.WriteF("gener.yaml", `
+	assert.NoError(t, os.Chmod(filepath.Join(tmpDir.String(), "exec.sh"), 0777))
+	th.WriteF(filepath.Join(tmpDir.String(), "gener.yaml"), `
 kind: executable
 metadata:
   name: demo
   annotations:
     config.kubernetes.io/function: |
       exec:
-        path: ./fnplugin_test/fnexectest.sh
+        path: ./exec.sh
 spec:
 `)
-	o := th.MakeOptionsPluginsEnabled()
-	o.PluginConfig.FnpLoadingOptions.EnableExec = true
-	m := th.Run(".", o)
-	th.AssertActualEqualsExpected(m, `
-apiVersion: v1
+
+	m := th.Run(tmpDir.String(), o)
+	assert.NoError(t, err)
+	yml, err := m.AsYaml()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: v1
 kind: Secret
 metadata:
   labels:
@@ -85,15 +118,16 @@ spec:
       containers:
       - image: nginx
         name: nginx
-`)
+`, string(yml))
+	assert.NoError(t, fSys.RemoveAll(tmpDir.String()))
 }
 
 func TestFnExecGeneratorWithOverlay(t *testing.T) {
 	fSys := filesys.MakeFsOnDisk()
-	th := kusttest_test.MakeHarness(t)
+
+	th := kusttest_test.MakeHarnessWithFs(t, fSys)
 	o := th.MakeOptionsPluginsEnabled()
 	o.PluginConfig.FnpLoadingOptions.EnableExec = true
-	b := krusty.MakeKustomizer(&o)
 
 	tmpDir, err := filesys.NewTmpConfirmedDir()
 	assert.NoError(t, err)
@@ -101,17 +135,18 @@ func TestFnExecGeneratorWithOverlay(t *testing.T) {
 	prod := filepath.Join(tmpDir.String(), "prod")
 	assert.NoError(t, fSys.Mkdir(base))
 	assert.NoError(t, fSys.Mkdir(prod))
-	assert.NoError(t, fSys.WriteFile(filepath.Join(base, "kustomization.yaml"), []byte(`
+	th.WriteK(base, `
 resources:
 - short_secret.yaml
 generators:
 - gener.yaml
-`)))
-	assert.NoError(t, fSys.WriteFile(filepath.Join(prod, "kustomization.yaml"), []byte(`
+`)
+	th.WriteK(prod, `
 resources:
 - ../base
-`)))
-	assert.NoError(t, fSys.WriteFile(filepath.Join(base, "short_secret.yaml"), []byte(`
+`)
+	th.WriteF(filepath.Join(base, "short_secret.yaml"),
+		`
 apiVersion: v1
 kind: Secret
 metadata:
@@ -123,34 +158,11 @@ stringData:
   userData: |
     bootcmd:
     - mkdir /mnt/vda
-`)))
-	assert.NoError(t, fSys.WriteFile(filepath.Join(base, "exec.sh"), []byte(`#!/bin/sh
+`)
+	th.WriteF(filepath.Join(base, "exec.sh"), execFile)
 
-cat <<EOF
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: nginx
-  labels:
-    app: nginx
-  annotations:
-    tshirt-size: small # this injects the resource reservations
-spec:
-  selector:
-    matchLabels:
-      app: nginx
-  template:
-    metadata:
-      labels:
-        app: nginx
-    spec:
-      containers:
-      - name: nginx
-        image: nginx
-EOF
-`)))
 	assert.NoError(t, os.Chmod(filepath.Join(base, "exec.sh"), 0777))
-	assert.NoError(t, fSys.WriteFile(filepath.Join(base, "gener.yaml"), []byte(`
+	th.WriteF(filepath.Join(base, "gener.yaml"), `
 kind: executable
 metadata:
   name: demo
@@ -159,15 +171,9 @@ metadata:
       exec:
         path: ./exec.sh
 spec:
-`)))
-
-	m, err := b.Run(
-		fSys,
-		prod)
-	if utils.IsErrTimeout(err) {
-		// Don't fail on timeouts.
-		t.SkipNow()
-	}
+`)
+
+	m := th.Run(prod, o)
 	assert.NoError(t, err)
 	yml, err := m.AsYaml()
 	assert.NoError(t, err)
diff --git a/api/krusty/fnplugin_test/fnexectest.sh b/api/krusty/fnplugin_test/fnexectest.sh
deleted file mode 100755
index d8e7c9eabb..0000000000
--- a/api/krusty/fnplugin_test/fnexectest.sh
+++ /dev/null
@@ -1,24 +0,0 @@
-#!/bin/sh
-
-cat <<EOF
-apiVersion: apps/v1
-kind: Deployment
-metadata:
-  name: nginx
-  labels:
-    app: nginx
-  annotations:
-    tshirt-size: small # this injects the resource reservations
-spec:
-  selector:
-    matchLabels:
-      app: nginx
-  template:
-    metadata:
-      labels:
-        app: nginx
-    spec:
-      containers:
-      - name: nginx
-        image: nginx
-EOF
diff --git a/api/resmap/resmap.go b/api/resmap/resmap.go
index b6b5118bda..7c0c2ea802 100644
--- a/api/resmap/resmap.go
+++ b/api/resmap/resmap.go
@@ -123,10 +123,6 @@ type ResMap interface {
 	// failing on any CurId collision.
 	AppendAll(ResMap) error
 
-	// AnnotateAll annotates all resources in the ResMap with
-	// the provided key value pair.
-	AnnotateAll(key string, value string) error
-
 	// AbsorbAll appends, replaces or merges the contents
 	// of another ResMap into self,
 	// allowing and sometimes demanding ID collisions.
diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go
index 2c25c65a61..391d11f523 100644
--- a/api/resmap/reswrangler.go
+++ b/api/resmap/reswrangler.go
@@ -545,19 +545,6 @@ func (m *resWrangler) appendReplaceOrMerge(res *resource.Resource) error {
 	}
 }
 
-// AnnotateAll implements ResMap
-func (m *resWrangler) AnnotateAll(key string, value string) error {
-	return m.ApplyFilter(annotations.Filter{
-		Annotations: map[string]string{
-			key: value,
-		},
-		FsSlice: []types.FieldSpec{{
-			Path:               "metadata/annotations",
-			CreateIfNotPresent: true,
-		}},
-	})
-}
-
 // Select returns a list of resources that
 // are selected by a Selector
 func (m *resWrangler) Select(s types.Selector) ([]*resource.Resource, error) {
diff --git a/kyaml/fn/runtime/exec/exec.go b/kyaml/fn/runtime/exec/exec.go
index fcd53d1512..7bab904c6a 100644
--- a/kyaml/fn/runtime/exec/exec.go
+++ b/kyaml/fn/runtime/exec/exec.go
@@ -32,18 +32,12 @@ func (c *Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 }
 
 func (c *Filter) Run(reader io.Reader, writer io.Writer) error {
-	if c.WorkingDir != "" {
-		p, err := os.Getwd()
-		if err != nil {
-			return err
-		}
-		os.Chdir(c.WorkingDir)
-		defer os.Chdir(p)
-	}
-
 	cmd := exec.Command(c.Path, c.Args...)
 	cmd.Stdin = reader
 	cmd.Stdout = writer
 	cmd.Stderr = os.Stderr
+	if c.WorkingDir != "" {
+		cmd.Dir = c.WorkingDir
+	}
 	return cmd.Run()
 }
diff --git a/kyaml/kio/kioutil/kioutil.go b/kyaml/kio/kioutil/kioutil.go
index 993cdfd848..16155e4b26 100644
--- a/kyaml/kio/kioutil/kioutil.go
+++ b/kyaml/kio/kioutil/kioutil.go
@@ -25,6 +25,10 @@ const (
 
 	// SeqIndentAnnotation records the sequence nodes indentation of the input resource
 	SeqIndentAnnotation AnnotationKey = "internal.config.kubernetes.io/seqindent"
+
+	// WorkingDirAnnotation records the directory of the kustomization that
+	// refers to the resource
+	WorkingDirAnnotation AnnotationKey = "internal.config.kubernetes.io/working-dir"
 )
 
 func GetFileAnnotations(rn *yaml.RNode) (string, string, error) {
diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go
index d3dd071d20..ca731490a0 100644
--- a/kyaml/runfn/runfn.go
+++ b/kyaml/runfn/runfn.go
@@ -507,15 +507,11 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode, currentUser
 	}
 
 	if r.EnableExec && spec.Exec.Path != "" {
-		currentDir, err := os.Getwd()
+		wd, err := getWorkingDirectory(api)
 		if err != nil {
 			return nil, err
 		}
 
-		wd, _, err := kioutil.GetFileAnnotations(api)
-		if err != nil || wd == "" || wd == "/" {
-			wd = currentDir
-		}
 		ef := &exec.Filter{
 			Path: spec.Exec.Path,
 			WorkingDir: wd,
@@ -530,3 +526,25 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode, currentUser
 
 	return nil, nil
 }
+
+func getWorkingDirectory(api *yaml.RNode) (string, error) {
+	annotations := api.GetAnnotations()
+	wd, ok := annotations[kioutil.WorkingDirAnnotation]
+	if !ok {
+		var err error
+		wd, err = os.Getwd()
+		if err != nil {
+			return "", err
+		}
+	} else {
+		if !filepath.IsAbs(wd) || !path.IsAbs(wd) {
+			return "", errors.Errorf(
+				"relative working directory %s not allowed", wd)
+		}
+		if wd == "/" {
+			return "", errors.Errorf(
+				"root working directory '/' not allowed")
+		}
+	}
+	return wd, nil
+}