feat: Add CEL-based conditional function execution (#4388)#4391
feat: Add CEL-based conditional function execution (#4388)#4391SurbhiAgarwal1 wants to merge 1 commit intokptdev:mainfrom
Conversation
SurbhiAgarwal1
requested review from
efiacor,
kispaljr and
mozesl-nokia
as code owners
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
SurbhiAgarwal1
force-pushed
the
feat/cel-conditional-execution
branch
from
32fe3c0 to
3d2bde5
Compare
There was a problem hiding this comment.
Did you mean to post these files on the issue rather than on the PR? It looks like AI generated content.
nagygergo
left a comment
nagygergo
left a comment
There was a problem hiding this comment.
Hey, good to see a new contributor.
Some general comments:
- Clean up AI instructions.
- Add e2e tests.
- Add documentation.
Some specific comments are inline.
|
|
||
| // NewCELEvaluator creates a new CEL evaluator with the standard environment | ||
| func NewCELEvaluator() (*CELEvaluator, error) { | ||
| env, err := cel.NewEnv( |
There was a problem hiding this comment.
This is a totally unprotected cel executor. There should be limitations on the number of CPU cycles it can consume, the amount of characters it can output, the max complexity of the ast.
| return NewFunctionRunner(ctx, fltr, pkgPath, fnResult, fnResults, opts) | ||
|
|
||
| // Initialize CEL evaluator if a condition is specified | ||
| var evaluator *CELEvaluator |
There was a problem hiding this comment.
Why do you need to create a new CEL env for each function evaluation? The env can be the same.
| pr := printer.FromContextOrDie(fr.ctx) | ||
|
|
||
| // Check condition before executing function | ||
| if fr.condition != "" && fr.evaluator != nil { |
There was a problem hiding this comment.
Why check if fr.evaluator exists or not. If the function runner was created with a condition appearing for it's Runner, then must have an evaluator. It's ok to run to a panic if it doesn't exist at this point.
| "github.com/stretchr/testify/require" | ||
| "sigs.k8s.io/kustomize/kyaml/yaml" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add a testcase that makes sure that the cel functions can't mutate the resourcelist that is the input. The function signature can allow for it, as it hands over the *yaml.RNode list.
|
|
||
| // Create function runner with condition | ||
| fnResult := &fnresult.Result{} | ||
| fnResults := &fnresult.ResultList{} |
There was a problem hiding this comment.
Are these needed for testware when initialising it?
| // NewCELEvaluator creates a new CEL evaluator with the standard environment | ||
| func NewCELEvaluator() (*CELEvaluator, error) { | ||
| env, err := cel.NewEnv( | ||
| cel.Variable("resources", cel.ListType(cel.DynType)), |
There was a problem hiding this comment.
Probably advanced strings libraries would be good to include. https://pkg.go.dev/github.com/google/cel-go/ext#Strings
| } | ||
|
|
||
| // Evaluate the expression | ||
| out, _, err := prg.Eval(map[string]interface{}{ |
There was a problem hiding this comment.
There should be a context passed to this to protect against long-hanging operations.
| } | ||
|
|
||
| // Convert resources to a format suitable for CEL | ||
| resourceList, err := e.resourcesToList(resources) |
There was a problem hiding this comment.
Is serialising all the yaml.RNode actually needed? As it's a map[string]any type anyways (with no strange subtypes), probably the CEL interpreter can deal with it directly. Serialising the whole package for the cel execution, then not reusing it can cause a significant memory footprint bloat.
3 participants
Implements #4388
Changes
conditionfield to Function schema for CEL expressionsExample Usage