render: changed relative path computing logic (#2295)

droot · web-flow · commit bce76b328e60 · 2021-06-28T16:25:37.000-07:00
The new logic uses an internal annotation to track the absolute path of package the resource belongs
and that is eventually used to write the resources on filesystem.
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.expected/config.yaml b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.expected/config.yaml
@@ -0,0 +1,18 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+testType: eval
+image: gcr.io/kpt-fn/set-namespace:v0.1.3
+args:
+  namespace: dev
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.expected/diff.patch b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.expected/diff.patch
@@ -0,0 +1,19 @@
+diff --git a/pkg-a/pkg-a/resources.yaml b/pkg-a/pkg-a/resources.yaml
+index 7a494c9..ba72b74 100644
+--- a/pkg-a/pkg-a/resources.yaml
++++ b/pkg-a/pkg-a/resources.yaml
+@@ -15,6 +15,7 @@ apiVersion: apps/v1
+ kind: Deployment
+ metadata:
+   name: nginx-deployment
++  namespace: dev
+ spec:
+   replicas: 3
+ ---
+@@ -22,5 +23,6 @@ apiVersion: custom.io/v1
+ kind: Custom
+ metadata:
+   name: custom
++  namespace: dev
+ spec:
+   image: nginx:1.2.3
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.krmignore b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/.krmignore
@@ -0,0 +1 @@
+.expected
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/Kptfile b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/Kptfile
@@ -0,0 +1,6 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: root-pkg
+info:
+  description: sample description
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/pkg-a/Kptfile b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/pkg-a/Kptfile
@@ -0,0 +1,6 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: pkg-a
+info:
+  description: sample description
diff --git a/e2e/testdata/fn-eval/subpkg-has-samename-subdir/pkg-a/pkg-a/resources.yaml b/e2e/testdata/fn-eval/subpkg-has-samename-subdir/pkg-a/pkg-a/resources.yaml
@@ -0,0 +1,26 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+spec:
+  replicas: 3
+---
+apiVersion: custom.io/v1
+kind: Custom
+metadata:
+  name: custom
+spec:
+  image: nginx:1.2.3
diff --git a/e2e/testdata/fn-render/empty-pipeline/Kptfile b/e2e/testdata/fn-render/empty-pipeline/Kptfile
@@ -2,4 +2,3 @@ apiVersion: kpt.dev/v1
 kind: Kptfile
 metadata:
   name: app
-pipeline:
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/.krmignore b/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/.krmignore
@@ -0,0 +1 @@
+.expected
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/Kptfile b/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/Kptfile
@@ -0,0 +1,6 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: root-pkg
+info:
+  description: sample description
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/pkg-a/Kptfile b/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/pkg-a/Kptfile
@@ -0,0 +1,6 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: pkg-a
+info:
+  description: sample description
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/pkg-a/pkg-a/resources.yaml b/e2e/testdata/fn-render/subpkg-has-samename-subdir-empty-pipeline/pkg-a/pkg-a/resources.yaml
@@ -0,0 +1,26 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+spec:
+  replicas: 3
+---
+apiVersion: custom.io/v1
+kind: Custom
+metadata:
+  name: custom
+spec:
+  image: nginx:1.2.3
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir/.expected/diff.patch b/e2e/testdata/fn-render/subpkg-has-samename-subdir/.expected/diff.patch
@@ -0,0 +1,19 @@
+diff --git a/pkg-a/pkg-a/resources.yaml b/pkg-a/pkg-a/resources.yaml
+index 7a494c9..ba72b74 100644
+--- a/pkg-a/pkg-a/resources.yaml
++++ b/pkg-a/pkg-a/resources.yaml
+@@ -15,6 +15,7 @@ apiVersion: apps/v1
+ kind: Deployment
+ metadata:
+   name: nginx-deployment
++  namespace: dev
+ spec:
+   replicas: 3
+ ---
+@@ -22,5 +23,6 @@ apiVersion: custom.io/v1
+ kind: Custom
+ metadata:
+   name: custom
++  namespace: dev
+ spec:
+   image: nginx:1.2.3
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir/.krmignore b/e2e/testdata/fn-render/subpkg-has-samename-subdir/.krmignore
@@ -0,0 +1 @@
+.expected
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir/Kptfile b/e2e/testdata/fn-render/subpkg-has-samename-subdir/Kptfile
@@ -0,0 +1,6 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: root-pkg
+info:
+  description: sample description
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir/pkg-a/Kptfile b/e2e/testdata/fn-render/subpkg-has-samename-subdir/pkg-a/Kptfile
@@ -0,0 +1,11 @@
+apiVersion: kpt.dev/v1
+kind: Kptfile
+metadata:
+  name: pkg-a
+info:
+  description: sample description
+pipeline:
+  mutators:
+    - image: gcr.io/kpt-fn/set-namespace:v0.1.3
+      configMap:
+        namespace: dev
diff --git a/e2e/testdata/fn-render/subpkg-has-samename-subdir/pkg-a/pkg-a/resources.yaml b/e2e/testdata/fn-render/subpkg-has-samename-subdir/pkg-a/pkg-a/resources.yaml
@@ -0,0 +1,26 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+spec:
+  replicas: 3
+---
+apiVersion: custom.io/v1
+kind: Custom
+metadata:
+  name: custom
+spec:
+  image: nginx:1.2.3
diff --git a/e2e/testdata/fn-render/validator-mutates-resources/Kptfile b/e2e/testdata/fn-render/validator-mutates-resources/Kptfile
@@ -4,6 +4,6 @@ metadata:
   name: app
 pipeline:
   validators:
-    - image: gcr.io/kpt-fn/set-labels:v0.1.4 # mutator function is being used as validator
+    - image: gcr.io/kpt-fn/set-labels:v0.1.4
       configMap:
         tier: backend
diff --git a/internal/cmdrender/executor.go b/internal/cmdrender/executor.go
@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 
@@ -65,29 +64,34 @@ func (e *Executor) Execute(ctx context.Context) error {
 		imagePullPolicy: e.ImagePullPolicy,
 	}
 
-	resources, err := hydrate(ctx, root, hctx)
-	if err != nil {
+	if _, err = hydrate(ctx, root, hctx); err != nil {
 		// Note(droot): ignore the error in function result saving
 		// to avoid masking the hydration error.
 		// don't disable the CLI output in case of error
 		_ = e.saveFnResults(ctx, hctx.fnResults)
 		return errors.E(op, root.pkg.UniquePath, err)
 	}
 
+	// adjust the relative paths of the resources.
+	err = adjustRelPath(hctx)
+	if err != nil {
+		return err
+	}
+
 	if err = trackOutputFiles(hctx); err != nil {
 		return err
 	}
 
 	// format resources before writing
-	_, err = filters.FormatFilter{UseSchema: true}.Filter(resources)
+	_, err = filters.FormatFilter{UseSchema: true}.Filter(hctx.root.resources)
 	if err != nil {
 		return err
 	}
 
 	if e.Output == nil {
 		// the intent of the user is to modify resources in-place
 		pkgWriter := &kio.LocalPackageReadWriter{PackagePath: string(root.pkg.UniquePath)}
-		err = pkgWriter.Write(resources)
+		err = pkgWriter.Write(hctx.root.resources)
 		if err != nil {
 			return fmt.Errorf("failed to save resources: %w", err)
 		}
@@ -105,7 +109,7 @@ func (e *Executor) Execute(ctx context.Context) error {
 			WrappingAPIVersion:    kio.ResourceListAPIVersion,
 			WrappingKind:          kio.ResourceListKind,
 		}
-		err = writer.Write(resources)
+		err = writer.Write(hctx.root.resources)
 		if err != nil {
 			return fmt.Errorf("failed to write resources: %w", err)
 		}
@@ -275,13 +279,7 @@ func hydrate(ctx context.Context, pn *pkgNode, hctx *hydrationContext) (output [
 		return output, errors.E(op, curr.pkg.UniquePath, err)
 	}
 
-	// ensure input resource's paths are relative to root pkg.
-	currPkgResources, err = adjustRelPath(currPkgResources, relPath)
-	if err != nil {
-		return nil, fmt.Errorf("adjust relative path: %w", err)
-	}
-
-	err = trackInputFiles(hctx, currPkgResources)
+	err = trackInputFiles(hctx, relPath, currPkgResources)
 	if err != nil {
 		return nil, err
 	}
@@ -294,12 +292,6 @@ func hydrate(ctx context.Context, pn *pkgNode, hctx *hydrationContext) (output [
 		return output, errors.E(op, curr.pkg.UniquePath, err)
 	}
 
-	// ensure generated resource's file path are relative to root pkg.
-	output, err = adjustRelPath(output, relPath)
-	if err != nil {
-		return nil, fmt.Errorf("adjust relative path: %w", err)
-	}
-
 	// pkg is hydrated, mark the pkg as wet and update the resources
 	curr.state = Wet
 	curr.resources = output
@@ -429,31 +421,72 @@ func cloneResources(input []*yaml.RNode) (output []*yaml.RNode) {
 }
 
 // path (location) of a KRM resources is tracked in a special key in
-// metadata.annotation field. adjustRelPath updates that path annotation by prepending
-// the given relPath to the current path annotation if it doesn't exist already.
-// Resources are read from local filesystem or generated at a package level, so the
-// path annotation in each resource points to path relative to that package.
-// But the resources are written to the file system at the root package level, so
+// metadata.annotation field that is used to write the resources to the filesystem.
+// When resources are read from local filesystem or generated at a package level, the
+// path annotation in a resource points to path relative to that package. But the resources
+// are written to the file system at the root package level, so
 // the path annotation in each resources needs to be adjusted to be relative to the rootPkg.
-func adjustRelPath(resources []*yaml.RNode, relPath string) ([]*yaml.RNode, error) {
-	if relPath == "" {
-		return resources, nil
-	}
+// adjustRelPath updates the path annotation by prepending the path of the package
+// a resource relative to the root package.
+func adjustRelPath(hctx *hydrationContext) error {
+	resources := hctx.root.resources
 	for _, r := range resources {
+		pkgPath, err := pkg.GetPkgPathAnnotation(r)
+		if err != nil {
+			return err
+		}
+		// Note: kioutil.GetFileAnnotation returns OS specific
+		// paths today, https://github.com/kubernetes-sigs/kustomize/issues/3749
 		currPath, _, err := kioutil.GetFileAnnotations(r)
 		if err != nil {
-			return resources, err
+			return err
+		}
+		newPath, err := pathRelToRoot(string(hctx.root.pkg.UniquePath), pkgPath, currPath)
+		if err != nil {
+			return err
 		}
-		// if currPath is relative to root pkg i.e. already has relPath, skip it
-		if !strings.HasPrefix(currPath, relPath+"/") {
-			newPath := path.Join(relPath, currPath)
-			err = r.PipeE(yaml.SetAnnotation(kioutil.PathAnnotation, newPath))
-			if err != nil {
-				return resources, err
-			}
+		if err = r.PipeE(yaml.SetAnnotation(kioutil.PathAnnotation, newPath)); err != nil {
+			return err
+		}
+		if err = pkg.RemovePkgPathAnnotation(r); err != nil {
+			return err
 		}
 	}
-	return resources, nil
+	return nil
+}
+
+// pathRelToRoot computes resource's path relative to root package given:
+// rootPkgPath: absolute path to the root package
+// subpkgPath: absolute path to subpackage
+// resourcePath: resource's path relative to the subpackage
+// All the inputs paths are assumed to be OS specific.
+func pathRelToRoot(rootPkgPath, subPkgPath, resourcePath string) (relativePath string, err error) {
+	if !filepath.IsAbs(rootPkgPath) {
+		return "", fmt.Errorf("root package path %q must be absolute", rootPkgPath)
+	}
+
+	if !filepath.IsAbs(subPkgPath) {
+		return "", fmt.Errorf("subpackage path %q must be absolute", subPkgPath)
+	}
+
+	if subPkgPath == "" {
+		// empty subpackage path means resource belongs to the root package
+		return resourcePath, nil
+	}
+
+	// subpackage's path relative to the root package
+	subPkgRelPath, err := filepath.Rel(rootPkgPath, subPkgPath)
+	if err != nil {
+		return "", fmt.Errorf("subpackage %q must be relative to %q: %w",
+			rootPkgPath, subPkgPath, err)
+	}
+	// Note: Rel("/tmp", "/a") = "../", which isn't valid for our use-case.
+	dotdot := ".." + string(os.PathSeparator)
+	if strings.HasPrefix(subPkgRelPath, dotdot) || subPkgRelPath == ".." {
+		return "", fmt.Errorf("subpackage %q is not a descendant of %q", subPkgPath, rootPkgPath)
+	}
+	relativePath = filepath.Join(subPkgRelPath, filepath.Clean(resourcePath))
+	return relativePath, nil
 }
 
 // fnChain returns a slice of function runners given a list of functions defined in pipeline.
@@ -471,7 +504,7 @@ func fnChain(ctx context.Context, hctx *hydrationContext, pkgPath types.UniquePa
 }
 
 // trackInputFiles records file paths of input resources in the hydration context.
-func trackInputFiles(hctx *hydrationContext, input []*yaml.RNode) error {
+func trackInputFiles(hctx *hydrationContext, relPath string, input []*yaml.RNode) error {
 	if hctx.inputFiles == nil {
 		hctx.inputFiles = sets.String{}
 	}
@@ -480,6 +513,7 @@ func trackInputFiles(hctx *hydrationContext, input []*yaml.RNode) error {
 		if err != nil {
 			return fmt.Errorf("path annotation missing: %w", err)
 		}
+		path = filepath.Join(relPath, filepath.Clean(path))
 		hctx.inputFiles.Insert(path)
 	}
 	return nil
diff --git a/internal/cmdrender/executor_test.go b/internal/cmdrender/executor_test.go
diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go
diff --git a/internal/pkg/pkg.go b/internal/pkg/pkg.go
diff --git a/pkg/test/runner/runner.go b/pkg/test/runner/runner.go
diff --git a/thirdparty/kyaml/runfn/runfn.go b/thirdparty/kyaml/runfn/runfn.go
