exec function working dir is the kustomization that referenced it #4125

natasha41575 · 2021-08-18T00:13:02Z

As discussed in #4117, this PR changes the working directory of exec functions to be the directory of the kustomization that referenced it.

@yuwenma

ALLOW_MODULE_SPAN

k8s-ci-robot · 2021-08-18T00:13:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

api/internal/target/kusttarget.go

natasha41575 · 2021-08-18T00:15:06Z

/retest

api/krusty/fnplugin_test.go

kyaml/runfn/runfn.go

kyaml/fn/runtime/exec/exec.go

api/internal/target/kusttarget.go

kyaml/runfn/runfn.go

k8s-ci-robot · 2021-08-18T20:50:08Z

@natasha41575: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

api/internal/target/kusttarget.go

KnVerey · 2021-08-18T21:38:28Z

kyaml/runfn/runfn.go

@@ -507,7 +507,16 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode, currentUser
 	}

 	if r.EnableExec && spec.Exec.Path != "" {
-		ef := &exec.Filter{Path: spec.Exec.Path}
+		annotations := api.GetAnnotations()
+		wd, ok := annotations[kioutil.WorkingDirAnnotation]


It occurs to me that this annotation could be set by the user, in theory. We should be overwriting it in all cases, but should we validate it just in case? i.e. throw an error if it doesn't "look like" a value we set--it's relative, or root. Or would it be an option to use an unserialized field on the FunctionSpec to more definitively prevent it from being settable by the user?

Not caused by this PR, but I'm surprised to notice that we're not doing any validation of the exec path at all. There are several restrictions on the Starlark path above.

I've added some validation to make sure its an absolute path, and not the root

use an unserialized field on the FunctionSpec to more definitively prevent it from being settable by the user

Do we need to definitively prevent the user from setting it? If the user really wants to change the working directory when running kustomize fn run, should we let them?

If so, what would that look like in the code? I may need some guidance as I've never seen that or tried to do that before.

Also, please see the above comment for why kustomize fn run needs to fall back on the current directory when this annotation is missing.

We shouldn't honor a user set value here. The file path annotation should be set only by kustomize as it reads.

That's what AnnotateAll does, so lgtm.

Regardless, sanity checks are always a good idea, because down the road someone will change the code.

Makes sense to fall back to cwd.

Just noting that AnnotateAll will overwrite anything the user put in, so we're good.

Just noting that AnnotateAll will overwrite anything the user put in, so we're good.

Unless there's a code path that doesn't run that, which as Natasha pointed out, is true of kustomize fn run--if I'm understanding correctly, the annotation would currently be honored in that code path, which we don't want.

If so, what would that look like in the code? I may need some guidance as I've never seen that or tried to do that before.

I think it would necessarily mean not using an annotation for this, but instead setting the root as a field on one of the function-related structs that are used for configuration. I'm not confident in my understanding of these code paths, but maybe the current kustomization root could be an option passed to loader creation in NewLoader within (b *Kustomizer) Run, then loadPlugin could set it on a FunctionSpec.WorkingDir field?

How important is it that kustomize fn run does not honor the annotation? What are the consequences of allowing the user to set the working dir for this case?

In any case I am looking into seeing how we might be able to do without annotations now. The solution you've proposed is probably viable; I'll push the change soon.

@monopole @KnVerey the most recent commit takes the suggested approach, and passes down the working directory through parameters and fields so that we don't have to worry about the user ever being able to set it themselves. PTAL

kyaml/runfn/runfn.go

api/krusty/fnplugin_test.go

api/resmap/reswrangler.go

KnVerey

👍 I like this approach better. Exec plugins carry inherent risk, so the more predictable and constrained we can make their invocation the better, IMO.

kyaml/runfn/runfn.go

api/internal/plugins/fnplugin/fnplugin.go

kyaml/runfn/runfn.go

monopole

/lgtm

thanks! holding this in the ldr smells right

KnVerey · 2021-08-20T15:17:41Z

kyaml/fn/runtime/exec/exec.go

@@ -32,5 +38,16 @@ func (c *Filter) Run(reader io.Reader, writer io.Writer) error {
 	cmd.Stdin = reader
 	cmd.Stdout = writer
 	cmd.Stderr = os.Stderr
+	if c.WorkingDir != "" {


Similar to the logic for having the path validations be here, right before we run the command, should the "working directory must be set" validation also go here Instead of in runfun.go?

The other entry point to the exec filter is the container runtime code, which uses the exec filter to run docker run. To do this validation here, we would have to set the working directory there was well.

Currently, the place in runfn.go, where I have the "working directory must be set" validation, is only used when running exec functions.

I will defer to your opinion on whether the container runtime code should set the working directory for exec filters as well. The PR to do so is here: #4132

seh · 2021-12-21T23:41:40Z

api/internal/target/kusttarget.go

@@ -40,11 +40,13 @@ func NewKustTarget(
 	validator ifc.Validator,
 	rFactory *resmap.Factory,
 	pLdr *loader.Loader) *KustTarget {
+	pLdrCopy := *pLdr
+	pLdrCopy.SetWorkDir(ldr.Root())


See #4350 for why this directory may not be the right choice.

natasha41575 requested a review from KnVerey August 18, 2021 00:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2021

k8s-ci-robot requested a review from phanimarupaka August 18, 2021 00:13

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021

natasha41575 commented Aug 18, 2021

View reviewed changes

api/internal/target/kusttarget.go Outdated Show resolved Hide resolved

monopole reviewed Aug 18, 2021

View reviewed changes

api/krusty/fnplugin_test.go Outdated Show resolved Hide resolved

monopole reviewed Aug 18, 2021

View reviewed changes

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

KnVerey reviewed Aug 18, 2021

View reviewed changes

kyaml/fn/runtime/exec/exec.go Outdated Show resolved Hide resolved

api/internal/target/kusttarget.go Outdated Show resolved Hide resolved

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

natasha41575 force-pushed the ExecFnWorkingDir branch from 34ba6f7 to 8569269 Compare August 18, 2021 20:51

natasha41575 requested review from monopole and KnVerey August 18, 2021 20:53

natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 18, 2021

natasha41575 force-pushed the ExecFnWorkingDir branch from 8569269 to 62077c3 Compare August 18, 2021 20:59

KnVerey reviewed Aug 18, 2021

View reviewed changes

api/internal/target/kusttarget.go Outdated Show resolved Hide resolved

KnVerey reviewed Aug 18, 2021

View reviewed changes

natasha41575 force-pushed the ExecFnWorkingDir branch from c9b622b to 3d2354f Compare August 18, 2021 22:29

natasha41575 requested a review from KnVerey August 18, 2021 22:49

exec function working dir is the kustomization that referenced it

e52bab4

natasha41575 force-pushed the ExecFnWorkingDir branch from 3d2354f to b85a648 Compare August 19, 2021 03:31

suggested changes

246c54a

natasha41575 force-pushed the ExecFnWorkingDir branch from b85a648 to 246c54a Compare August 19, 2021 03:42

monopole reviewed Aug 19, 2021

View reviewed changes

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

api/krusty/fnplugin_test.go Outdated Show resolved Hide resolved

api/krusty/fnplugin_test.go Outdated Show resolved Hide resolved

monopole reviewed Aug 19, 2021

View reviewed changes

api/resmap/reswrangler.go Outdated Show resolved Hide resolved

more code review

eb3493c

natasha41575 requested a review from monopole August 19, 2021 21:19

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021

natasha41575 force-pushed the ExecFnWorkingDir branch from 122d708 to b7936cc Compare August 19, 2021 21:38

natasha41575 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021

natasha41575 force-pushed the ExecFnWorkingDir branch from b7936cc to 54613b9 Compare August 19, 2021 21:39

use a field instead of an annotation

16482c9

natasha41575 force-pushed the ExecFnWorkingDir branch from 54613b9 to 16482c9 Compare August 19, 2021 21:42

KnVerey reviewed Aug 19, 2021

View reviewed changes

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

api/internal/plugins/fnplugin/fnplugin.go Outdated Show resolved Hide resolved

kyaml/runfn/runfn.go Outdated Show resolved Hide resolved

natasha41575 requested a review from KnVerey August 19, 2021 22:11

natasha41575 force-pushed the ExecFnWorkingDir branch 3 times, most recently from 88fd52a to b9183a3 Compare August 19, 2021 22:20

more code review

05228bb

natasha41575 force-pushed the ExecFnWorkingDir branch from b9183a3 to 05228bb Compare August 19, 2021 22:30

monopole reviewed Aug 20, 2021

View reviewed changes

k8s-ci-robot assigned monopole Aug 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2021

k8s-ci-robot merged commit 1e1b9b4 into kubernetes-sigs:master Aug 20, 2021

KnVerey reviewed Aug 20, 2021

View reviewed changes

natasha41575 mentioned this pull request Aug 20, 2021

move check for working dir for exec functions #4132

Merged

galexeyev mentioned this pull request Aug 26, 2021

Exec plugins with argsFromFile can look for the file in wrong locations #4115

Closed

natasha41575 mentioned this pull request Aug 27, 2021

Exec functions look for files in the wrong location #4117

Closed

KnVerey mentioned this pull request Sep 22, 2021

Reference Starlark Function #4126

Closed

seh mentioned this pull request Dec 21, 2021

Exec KRM functions with relative paths don't resolve when base kustomization is in use #4350

Closed

seh reviewed Dec 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec function working dir is the kustomization that referenced it #4125

exec function working dir is the kustomization that referenced it #4125

natasha41575 commented Aug 18, 2021 •

edited

k8s-ci-robot commented Aug 18, 2021

natasha41575 commented Aug 18, 2021

k8s-ci-robot commented Aug 18, 2021

KnVerey Aug 18, 2021

natasha41575 Aug 18, 2021 •

edited

monopole Aug 19, 2021

monopole Aug 19, 2021

KnVerey Aug 19, 2021

natasha41575 Aug 19, 2021 •

edited

natasha41575 Aug 19, 2021 •

edited

KnVerey left a comment

monopole left a comment

KnVerey Aug 20, 2021

natasha41575 Aug 20, 2021

seh Dec 21, 2021

exec function working dir is the kustomization that referenced it #4125

exec function working dir is the kustomization that referenced it #4125

Conversation

natasha41575 commented Aug 18, 2021 • edited

k8s-ci-robot commented Aug 18, 2021

natasha41575 commented Aug 18, 2021

k8s-ci-robot commented Aug 18, 2021

KnVerey Aug 18, 2021

Choose a reason for hiding this comment

natasha41575 Aug 18, 2021 • edited

Choose a reason for hiding this comment

monopole Aug 19, 2021

Choose a reason for hiding this comment

monopole Aug 19, 2021

Choose a reason for hiding this comment

KnVerey Aug 19, 2021

Choose a reason for hiding this comment

natasha41575 Aug 19, 2021 • edited

Choose a reason for hiding this comment

natasha41575 Aug 19, 2021 • edited

Choose a reason for hiding this comment

KnVerey left a comment

Choose a reason for hiding this comment

monopole left a comment

Choose a reason for hiding this comment

KnVerey Aug 20, 2021

Choose a reason for hiding this comment

natasha41575 Aug 20, 2021

Choose a reason for hiding this comment

seh Dec 21, 2021

Choose a reason for hiding this comment

natasha41575 commented Aug 18, 2021 •

edited

natasha41575 Aug 18, 2021 •

edited

natasha41575 Aug 19, 2021 •

edited

natasha41575 Aug 19, 2021 •

edited