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

New Starlark fn does not support custom parameters nor imperative run #1889

Closed
yhrn opened this issue May 4, 2021 · 21 comments
Closed

New Starlark fn does not support custom parameters nor imperative run #1889

yhrn opened this issue May 4, 2021 · 21 comments
Assignees
Labels
area/fn-catalog Functions Catalog customer deep engagement enhancement New feature or request size/L 4 day triaged Issue has been triaged by adding an `area/` label
Milestone

Comments

@yhrn
Copy link

yhrn commented May 4, 2021

Apologies if this is premature feedback - I realize that this is yet to be released officially. Feel free to close if this is already planned for.

I tried out the new Starlark fn and it's pretty limited compared to the current built in alpha functionality:

  1. There is no way to pass parameters to the Starlark code, the function requires a custom config that only takes a single parameter source
  2. Because it requires a custom config it can't be run imperatively.

What I would like is to be able to do something like:

kpt fn run . --image gcr.io/kpt-fn/starlark:unstable -- source="$(cat my-func.star)" some-param=xyz

Less important, but it would also be nice to be able to use a custom config structure that gets passed into the starlark function (in ctx.resource_list["functionConfig"]) like you can today with the Starlark runtime

@yhrn yhrn added the enhancement New feature or request label May 4, 2021
@mikebz mikebz added the customer deep engagement label May 4, 2021
@mikebz mikebz added this to To do in kpt kanban board via automation May 4, 2021
@mikebz mikebz added this to the v1.0 m3 milestone May 4, 2021
@mikebz mikebz added the area/fn-catalog Functions Catalog label May 4, 2021
@mikebz
Copy link
Contributor

mikebz commented May 4, 2021

@mengqiy heads up

@mikebz mikebz added the triaged Issue has been triaged by adding an `area/` label label May 4, 2021
@mengqiy
Copy link
Contributor

mengqiy commented May 5, 2021

@yhrn Thanks a lot for your feedbacks!

There is no way to pass parameters to the Starlark code, the function requires a custom config that only takes a single parameter source

This is planed to be solved in #1560.

Because it requires a custom config it can't be run imperatively.

To support it, we will need to support ConfigMap as fn config. We will share the design with you when we have something.

@mengqiy mengqiy added the size/L 4 day label May 26, 2021
@mikebz mikebz modified the milestones: v1.0 m3, v1.0 m4 Jun 9, 2021
@mengqiy
Copy link
Contributor

mengqiy commented Jun 16, 2021

This is becoming a blocker for Spotify: #1834 (comment)
There are a few things planned for starlark function:

Re. supporting imperative run, @frankfarzan I remember you have some thoughts. Can you please elaborate?

@mengqiy
Copy link
Contributor

mengqiy commented Jun 16, 2021

@yhrn Can you please explain a bit why imperative run is important to you?
If the developer experience is the reason, we can provide a small tool to wrap the starlark script for you.
The UX would be something like:

$ wrapStarlarkScript --input=path/to/source.star --output=path/to/starlark-fn-config.yaml
$ kpt fn eval --image gcr.io/kpt-fn/startlark:v0.1 --fn-config path/to/starlark-fn-config.yaml

Is that reasonable to you?

@frankfarzan
Copy link
Contributor

frankfarzan commented Jun 16, 2021

wrapStarlarkScript can also be a preprocessor function:

# Expand StarlarkRun resource
$ kpt fn eval --image gcr.io/kpt-fn/generate-starlark-run --mount ...
# Run starklark function with expanded StarlarkRun (Or alternatively run starlark using eval)
$ kpt fn render

You only need to run the generate function when there's a change in the starlark script.

Alternatively, there can be a generic function that can expand KRM fields pointing to file contents
which can be used for other functions like gatekeeper:

# Expand KRM fields referencing (via a setter-like comment) a file.
# This can only be run imperatively.
$ kpt fn eval --image gcr.io/kpt-fn/file-substitution --mount ...
# Run starlark function with expanded StarlarkRun
$ kpt fn render

This option is more generally applicable to executive configuration pattern:

https://kpt.dev/book/05-developing-functions/04-executable-configuration

Both of these can be done without changing kpt CLI, as they're purely optional porcelain.

If the use case is to run Starlark with eval and provide the star script using CLI arguments, you can also have starlark function effectively embed and invoke generate-starlark-run where the fnConfig is generated on the fly (or rather, there's no fnConfig in that case). Obviously, you can't use this with render. This approach doesn't make sense for gatekeeper since the policies are also used server-side, so you actually want the expanded KRM resource.

@yhrn
Copy link
Author

yhrn commented Jun 17, 2021

Imperative run is important to us because we often do out-of-place rendering. Also, having a raw *.star file significantly improves the developer experience. I suppose the wrapper function idea works but I don't think it is very intuitive and it would also need to support passing parameters to the actual function.

Compare the suggested workaround to:

kpt fn run --enable-star --star-path fn.star -- foo=bar

@mengqiy mengqiy modified the milestones: v1.0 m4, v1.0 m3 Jun 17, 2021
@mengqiy mengqiy moved this from To do to In progress in kpt kanban board Jun 17, 2021
@mengqiy
Copy link
Contributor

mengqiy commented Jun 25, 2021

I have a proposal here:

functionConfig

Add a new field called parameters in StarlarkRun functionConfig to allow users to provide custom parameters. parameters is a map from string to string.

An example StarlarkRun:

apiVersion: fn.kpt.dev/v1alpha1
kind: StarlarkRun
metadata:
  name: set-namespace-to-prod
parameters:
  namespace: prod
source: |
  def setnamespace(resources, namespace):
    for resource in resources:
      resource["metadata"]["namespace"] = namespace
  ns_value = ctx.resource_list["functionConfig"]["parameters"]["namespace"] # parameters can be accessed like this
  setnamespace(ctx.resource_list["items"], ns_value)

Developer Experience

First, user needs to install the starlark binary.

$ go get github.com/GoogleContainerTools/kpt-functions-catalog/functions/go/starlark@<the-desired-version>

Users are going to develop and iterate on this starlark script using the following command.

$ kpt fn eval --exec "starlark path/to/sourcefile.star"  -- param1=foo param2=bar

Running the above command is equivalent to the following:

$ kpt fn eval --image gcr.io/kpt-fn/starlark:v0.1 --fn-config starlark-fn-config.yaml

where starlark-fn-config.yaml is:

apiVersion: fn.kpt.dev/v1alpha1
kind: StarlarkRun
metadata:
  name: starlark-fn-config
parameters:
  param1: foo
  param2: bar
source: |
  # content of sourcefile.star
  starlark source comes here ...

To generate a StarlarkRun resource with source and parameters.
Option 1: Use function gcr.io/kpt-fn/gen-starlark-run

$ kpt fn eval --image gcr.io/kpt-fn/gen-starlark-run:v0.1 --mount type=bind,src=/absolute/path/to/starlarksource,dst=/starlarksource -- param1=foo param2=bar

Option 2: Use function file-substitution. Details TBD.

Implementation Details

Changes to starlark binary

Currently the starlark binary (github.com/GoogleContainerTools/kpt-functions-catalog/functions/go/starlark) is only used in the starlark docker image. We are going to allow user to use it as a binary in development.

  • We need to make it go getable.
  • Allow the parameters field in StarlarkRun
  • It currently doesn't accept any arguments. We need to change it to accept one optional argument which is the path to a raw starlark source. And when the argument is provided, the starlark binary also accept a ConfigMap to provide parameters.

@mengqiy
Copy link
Contributor

mengqiy commented Jun 25, 2021

@yhrn WDYT?

@bzub
Copy link
Contributor

bzub commented Jun 25, 2021

Seems like the current implementation as well as the proposed enhancements/developer workflow are still comprised of key/value string pairs. Would it be a bad pattern if StarlarkRun used a ConfigMap for fnconfig, and reserved certain keys or key prefixes or suffixes for things like source (rename the key to main.star?), and all other data elements are used as "parameters" for the script? Could the function update its own config file if it finds a starlark script file on disk with a name that matches (main.star for example), and otherwise uses what is already in the fnconfig?

@mengqiy
Copy link
Contributor

mengqiy commented Jun 25, 2021

It sounds like you are suggesting something like this:

kpt fn eval --image gcr.io/kpt-fn/starlark:v0.1 -- source="$(cat my-func.star)" param1=foo param2=bar

ConfigMap as a functionConfig is designed to host simple key-value pairs only. If we want to do more than that, we should use CRD.
It may be debatable that if reserve key source and make it accept a multi-line string is consider simple key-value pairs.

@droot @frankfarzan Thoughts?

@yhrn
Copy link
Author

yhrn commented Jun 28, 2021

Regarding the proposal I think that it addresses the most important blocker from my PoW by supporting simple parameters. That said also think that this is still a significant regression compared to the built in Starlark runtime:

  • The developer UX is still significantly more complicated and there is a disconnect between what you run when iterating on your code and what you run "in prod". And this is IMO a pretty big deal - as a KRM advocate within our org I'm constantly faced with engineers being fatigued from learning all the new concepts and tools. The v0 Starlark functionality was a fantastic way of showing off kpt fn functionality and making it more accessible but I think that goes away with v1.
  • No support for complex fn params means that we can't use Starlark for writing a fn PoC using the intended API (client side CRD) and then later, possibly switch to a runtime more suitable for complex functions.

All in all, providing Starlark as a fn rather than a supported runtime just seems to give a lower fidelity experience.

@mengqiy
Copy link
Contributor

mengqiy commented Jun 29, 2021

We can support ConfigMap as the functionConfig unless I hear some major objection.

kpt fn eval --image starlark:v0.1 -- source=$(cat main.star) key1=val1 key2=val2

This should provide a simple enough developer experience. A developer can keeper iterate on it until the starlark script is ready.

After that the user may want to run the script with StarlarkRun as the functionConfig.
Current imperative UX:

  1. Edit myscript.star
  2. Copy-paste it into myscript.yaml
  3. Run kpt fn eval --image starlark:v0.1 --fn-config myscript.yaml

We can automate it a bit.
Assume we have a function called include-file, it replaces fields using the content from a file. This function can be useful for stalark and gatekeeper. It can be yaml comment driven substitution.
Then the workflow will be:

  1. Edit myscript.star
  2. Run kpt fn eval --image include-file --mount ...
  3. Run kpt fn eval --image starlark:v0.1 --fn-config myscript.yaml

We can alternatively make starlark smarter. The starlark function will look the starlark script file in a specific location. If found, the starlark function will execute that script. A user can choose to mount the starlark file in the starlark container.
The workflow will be:

  1. Edit myscript.star
  2. Run kpt fn eval --image starlark:v0.1 --mount ...

Current declarative UX:

  1. Edit myscript.star
  2. Copy-paste it into myscript.yaml
  3. Run kpt fn render and the Kptfile points to myscript.yaml as the functionConfig.

We can automate it a bit:
Assume include-file is same as mentioned above.
The workflow will be:

  1. Edit myscript.star
  2. Run kpt fn eval --image include-file --mount ...
  3. Run kpt fn render and the Kptfile points to myscript.yaml as the functionConfig.

@yhrn @bzub Thought about the above ideas?

@yhrn
Copy link
Author

yhrn commented Jun 29, 2021

Being able to do kpt fn eval --image starlark:v0.1 -- source=$(cat main.star) key1=val1 key2=val2 would address my biggest remaining concern.

I guess the concern not addressed is that there is no way to pass a complex structure as configuration to the Starlark code. It means that you can no longer treat is as (mostly) an implementation detail if you choose to write your fn using Starlark or Go/Typescript, but maybe that is not a too common use case.

@mengqiy
Copy link
Contributor

mengqiy commented Jun 29, 2021

It's possible to support complex structure (which can be present by yaml) in params field in StarlarkRun. But we can't do it for ConfigMap. Otherwise, we are abusing ConfigMap.

@yhrn
Copy link
Author

yhrn commented Jun 29, 2021

It's possible to support complex structure (which can be present by yaml) in params field in StarlarkRun. But we can't do it for ConfigMap. Otherwise, we are abusing ConfigMap.

Yes, that makes sense and I didn't mean that it should be supported for the ConfigMap case. It's not supported for the imperative case in v0 either so it wouldn't be worse from that perspective either.

If it's supported for the StarlarkRun case then I'm happy.

@frankfarzan
Copy link
Contributor

Created this issue for the include-file function that's generally useful: #2350

@mengqiy
Copy link
Contributor

mengqiy commented Jul 7, 2021

@mengqiy mengqiy closed this as completed Jul 7, 2021
kpt kanban board automation moved this from In progress to Done Jul 7, 2021
@yhrn
Copy link
Author

yhrn commented Jul 8, 2021

@mengqiy shouldn't there be a release of the starlark kpt fn too before this is fully done? Right now the new functionality can only be accessed via the unstable tag.

@bgrant0607
Copy link
Contributor

With the code in a ConfigMap, is it possible to inline the Starlark code and parameters into the Kptfile?

@mikebz mikebz added this to the v1.0.0-beta.2 milestone Jul 14, 2021
@mengqiy
Copy link
Contributor

mengqiy commented Jul 27, 2021

@yhrn In case you missed it, the starlark:v0.2.0 function has been released.

@mengqiy
Copy link
Contributor

mengqiy commented Jul 27, 2021

With the code in a ConfigMap, is it possible to inline the Starlark code and parameters into the Kptfile?

@bgrant0607 It's possible. But I guess it's not recommended unless the starlark script is very short. Otherwise, it may make the Kptfile a little verbose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fn-catalog Functions Catalog customer deep engagement enhancement New feature or request size/L 4 day triaged Issue has been triaged by adding an `area/` label
Projects
Development

No branches or pull requests

6 participants