Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exec functions look for files in the wrong location #4117

Closed
natasha41575 opened this issue Aug 12, 2021 · 16 comments
Closed

Exec functions look for files in the wrong location #4117

natasha41575 opened this issue Aug 12, 2021 · 16 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@natasha41575
Copy link
Contributor

natasha41575 commented Aug 12, 2021

I have the following directory structure:

├── base
│   ├── kustomization.yaml
│   └── local-files
├── kustomization.yaml
└── overlay
    └── kustomization.yaml

In my base/kustomization.yaml, I have something like:

generators:
- |-
  apiVersion: v1
  kind: FunctionKind
  metadata:
    name: demo
    annotations:
      config.kubernetes.io/function: |
        exec:
          path: /path/to/function/binary
  functionFiles:
  - local-files

My function needs to read local files. However, when running kustomize build, I find that when looking up local files, the working directory for the function is wherever kustomize build is run.

I believe the working directory for the function should instead be the directory of the function configuration - base in my case.

As a workaround, I can do:

generators:
- |-
  apiVersion: v1
  kind: FunctionKind
  metadata:
    name: demo
    annotations:
      config.kubernetes.io/function: |
        exec:
          path: /path/to/function/binary
  functionFiles:
  - base/local-files

but this makes my base configuration nonreusable as my base now needs to know the the relative path to itself from wherever kustomize build is run.

@natasha41575 natasha41575 added the kind/bug Categorizes issue or PR as related to a bug. label Aug 12, 2021
@k8s-ci-robot
Copy link
Contributor

@natasha41575: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 12, 2021
@mikebz
Copy link
Contributor

mikebz commented Aug 17, 2021

interested to hear what others think here, but I do agree that enabling relative paths helps people assemble a bigger kustomization projects without having to go and patch up subdirectories.

@natasha41575
Copy link
Contributor Author

natasha41575 commented Aug 17, 2021

enabling relative paths helps people assemble a bigger kustomization projects without having to go and patch up subdirectories

One important note is that I don't think I can use a kustomize patch with my configuration. I have to manually update the base, because kustomize can't target fields in inline strings (or if it can, I don't know the syntax), and I'm also unsure if kustomize can patch a base kustomization file?

So for my case, I have to manually change the base configuration. This won't work with remote bases at all, and isn't scalable if the base needs to be reused across many kustomizations with even slightly different directory structures.

Even if I could patch it with kustomize, there is another problem. If my top-level kustomization is in directory $DIR/example, and I am in $DIR, and I run

$ kustomize build example --enable-alpha-plugins --enable-exec

the exec function in my base looks for the local files in $DIR - outside of the root kustomization directory.

If I cd into example, however:

$ cd example
$ kustomize build . --enable-alpha-plugins --enable-exec

the exec function looks for the local files in $DIR/example.

So - the function is looking for the files wherever kustomize build is run. It would be one thing if it were always relative to the top-level kustomization, but the fact that it can look for local files outside of the kustomization directory and that it changes depending on where you run kustomize build is a big problem.

@natasha41575
Copy link
Contributor Author

@KnVerey let me know if you have any thoughts. I'm hoping to look into fixing this soon.

@KnVerey
Copy link
Contributor

KnVerey commented Aug 17, 2021

Re: patching transformer configuration, it is possible by treating the config as a resource in the lower layers. It's awkward though. Composition is designed around improving this, and the KEP comes with examples: https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/2299-kustomize-plugin-composition/example

That said, I agree that the behaviour you're describing is unintuitive, and that end users shouldn't be required to use patches to fix relative paths as they build up a configuration tree. Although it is infeasible to limit where exec plugins access files, I think we should optimize for the best practice case you're describing, i.e. where the files in question are below the Kustomization root.

Thinking out loud, we could:

  • Have the exec plugin runtime change directories into either the directory of the Kustomization that referenced the transformer config, or the directory of the transformer config itself, which may be different. We could drive this with an annotation, such that the transformer config itself could override the default (in which case we could validate that the given location complies with the current load restrictor). This could get confusing when the transformer config is patched, i.e. the original config location may be different from the location of a patch that added a field containing a path--this could be common in Composition.
  • Resolve paths in transformer configuration when we load it. The exec plugin is then invoked from the Kustomization root, but all the paths in the configuration it receives are absolute. This could be complicated/expensive, and might result in undesirable resolution of paths that are not referring to local context (e.g. a Logger function is taking an application's container log output path as input).
  • Same as the above, but only a specific field (e.g. .spec.localFilePaths). That field then becomes reserved, in the sense that although the function can do whatever it wants with the value, its type must be []string (or maybe map[string]string?) if present. A wrinkle to address would be how to ensure this behaves intuitively for starlark (same as exec, or maybe n/a?) and container (need to check up on how mount specification works) plugins.

@natasha41575
Copy link
Contributor Author

natasha41575 commented Aug 17, 2021

Have the exec plugin runtime change directories into either the directory of the Kustomization that referenced the transformer config, or the directory of the transformer config itself, which may be different. We could drive this with an annotation, such that the transformer config itself could override the default (in which case we could validate that the given location complies with the current load restrictor). This could get confusing when the transformer config is patched, i.e. the original config location may be different from the location of a patch that added a field containing a path--this could be common in Composition.

This is what I was thinking and seems to be the simplest and most intuitive solution to me of the three options you've written. My vote is for it to change directories into the directory of the kustomization that referenced the transformer config; I think it more makes sense for paths to be relative to the kustomization that invoked the function.

@KnVerey
Copy link
Contributor

KnVerey commented Aug 17, 2021

How do you propose we handle the situation where the transformer config originates in a different layer from the one where it is executed (either because it is manipulated as a resource before being used as a transformer, or because it's in a multi-level Composition)? I'm not sure that can be solved without resorting to some path resolution feature or another. I suppose the two aren't mutually exclusive, and I do agree that having the working dir be that of the "current" Kustomization makes sense. I can propose a path resolution feature in the future when Composition makes the need for it more obvious by making it easy to patch transformer config.

@natasha41575
Copy link
Contributor Author

natasha41575 commented Aug 17, 2021

How do you propose we handle the situation where the transformer config originates in a different layer from the one where it is executed (either because it is manipulated as a resource before being used as a transformer, or because it's in a multi-level Composition)?

I don't have a strong opinion, but maybe that is because I don't fully understand the details of this situation. IIUC the options are:

  1. Leave the file paths in the transformer config as is; the author of the underlying transformer config will have to specify paths that are relative to the kustomization which refers to it.
  2. Your option 2 - Resolve the paths of the transformer config when it is loaded.

Is that correct?

A question I have is, how will a base kustomization that is manipulating the transformer config as a resource before it is used as a transformer know (a) that it is a transformer config, and (b) which of its fields are intended to be filepaths? Is your option 3 intended to solve this?

As a side note, I'd like to submit a PR to change the exec function's working dir to be the current kustomization. I believe that can be done independently of this discussion regarding transformer configs.

@KnVerey
Copy link
Contributor

KnVerey commented Aug 18, 2021

Leave the file paths in the transformer config as is; the author of the underlying transformer config will have to specify paths that are relative to the kustomization which refers to it.

Almost. I think the paths would have to be relative to the kustomization that uses the config as a transformer. Which means that the lower layers that introduce and manipulate the config as a resource would need to write paths relative to the higher level config that imports it into the transformers field--very weird. As the Composition KEP sets out, this workflow isn't very usable in any case, so maybe this is just one more awkward thing about it.

A question I have is, how will a base kustomization that is manipulating the transformer config as a resource before it is used as a transformer know (a) that it is a transformer config, and (b) which of its fields are intended to be filepaths? Is your option 3 intended to solve this?

You're right, the base kustomization will have no idea it is a transformer config. Option 3 might kinda work, but could have unintended consequences when applied to the resources field. In Kustomization, we should probably leave it alone.

We should have a way for this to work nicely/intuitively in Composition though, since modifying transformer config is a first-class operation for it. In that case, we know for sure that what we're manipulating is transformer config, so option 3 becomes more viable.

Another idea could be to have an annotation drive path resolution (e.g. the annotation lists jsonpaths in the current object/smp that should be resolved, and it is stripped immediately before accumulation/application). That could be annoying for the end user, since the annotation would have to be included on every single instance of the transformer. If we had the schema for the transformer config (CRD analogue), that could contain open api meta identifying fields with paths that should be resolved.

As a side note, I'd like to submit a PR to change the exec function's working dir to be the current kustomization. I believe that can be done independently of this discussion regarding transformer configs.

Agreed, we have to have some working directory for exec functions regardless, and I think your proposal that it be the current Kustomization makes more sense.

@natasha41575
Copy link
Contributor Author

natasha41575 commented Aug 27, 2021

My intent with filing this issue has been resolved by #4125, but should we keep this issue open (potentially retitling it) for the discussion on transformer configs?

Regarding your comment:

In that case, we know for sure that what we're manipulating is transformer config, so option 3 becomes more viable.

For container functions, does Composition propose any UX for mounting files into a directory? The kustomize function spec provides support for a field mounts. Quoting from the KEP:

Function invocation machinery will be built using the existing sigs.k8s.io/kustomize/kyaml/fn/runtime package.

To me this implies that Composition can support the mounts field relatively easily. Is Composition planning to leverage this? The source mount path would have to be resolved at some point as well, whether it's the layer it's defined in or the layer it is executed at. How would mounts.src interact with spec.localFilePaths?

Starlark support likely depends on whether we decide to support starlark runtime or run starlark functions as a containerized KRM function, e.g. via https://catalog.kpt.dev/starlark/v0.2/.

Another idea could be to have an annotation drive path resolution (e.g. the annotation lists jsonpaths in the current object/smp that should be resolved, and it is stripped immediately before accumulation/application). That could be annoying for the end user, since the annotation would have to be included on every single instance of the transformer. If we had the schema for the transformer config (CRD analogue), that could contain open api meta identifying fields with paths that should be resolved.

This option also seems feasible but I think I prefer option 3 from above. Local file paths seems more intuitive to me as part of the function spec than an additional annotation. Though another interesting thing with this idea is that a Kustomization could also use the existence of these annotations to know when a resource it is modifying is intended to be a transformer config.

@KnVerey
Copy link
Contributor

KnVerey commented Aug 30, 2021

I've opened a new issue to continue the discussion: #4154. Let's close this one.

For container functions, does Composition propose any UX for mounting files into a directory?

It doesn't propose anything in particular. The differences from Kustomization are mainly in how the config itself is orchestrated, not what it contains. A minor exception is that it's proposing graduating an annotation to a reserved field.

Function invocation machinery will be built using the existing sigs.k8s.io/kustomize/kyaml/fn/runtime package.

Implementation is TBD, but at this point I'm actually hoping to reuse as much of Kustomization's function invocation code path as possible, rather than a second implementation on top of the kyaml runtime. FWIW.

Though another interesting thing with this idea is that a Kustomization could also use the existence of these annotations to know when a resource it is modifying is intended to be a transformer config.

It could be a decent signal, yeah, though requiring the local-config annotation might be a better one? As described the feature would support resolving paths in any resource, though I dunno what the point would be within a deployed resource.

@KnVerey KnVerey closed this as completed Aug 30, 2021
@seh
Copy link
Contributor

seh commented Dec 21, 2021

I find that this still doesn't work for me in kustomize version 4.4.1, even with #4125 merged. kustomize appears to use the directory in which I invoke it as the working directory for interpreting relative function paths, as opposed to the directory of the containing kustomization as promised.

Absolute paths to functions work fine, and relative paths "work" if I happen to situate my shell in the proper directory before invoking kustomize. Varying my working directory causes these relative paths to the function files to no longer work.

@KnVerey
Copy link
Contributor

KnVerey commented Dec 21, 2021

I just tried reproducing with 4.4.1, and the paths are resolved relative to the parent Kustomization as intended. Please note that this fix went in for exec functions specifically (not legacy exec, nor starlark plugins). If you find that is not the case, please open a new issue with detailed reproduction steps.

@seh
Copy link
Contributor

seh commented Dec 21, 2021

Can you please share with me both the path you wrote into your "metadata.annotations.config\.kubernetes\.io/function" annotation's "exec.path" field? Also, to be clear, are you running something like kustomize fn run ./overlays/example --enable-exec?

@seh
Copy link
Contributor

seh commented Dec 21, 2021

I think I found the problem: If the kustomization containing the function references any other kustomizations as base resources (that is, an entry in the "resources" field is directory), then these relative paths don't work. The "Exec KRM functions" example does not exercise this case.

I'll file an issue to that effect.

@seh
Copy link
Contributor

seh commented Dec 21, 2021

Please see #4350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

No branches or pull requests

5 participants