feat: Add CEL-based conditional function execution (#4388)#4469
feat: Add CEL-based conditional function execution (#4388)#4469SurbhiAgarwal1 wants to merge 1 commit intokptdev:mainfrom
Conversation
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
2037fa3 to
b9a1ab1
Compare
|
Comment from #4391: Closing this PR in favor of a clean rebase. The branch had accumulated 61 commits including upstream commits from other contributors, making it messy to review. All the feedback from this review has been addressed in the new PR which has a single clean commit: Thank you all for the thorough review! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
thirdparty/kyaml/runfn/runfn.go:1
NewFunctionRunner’s error is discarded andSetConditionis called without validatingopts.CELEnvironment. This can both (a) mask construction failures and (b) silently ignore the condition at runtime whenCELEnvironmentis nil (sinceFilter()only evaluates conditions whencelEnv != nil). Handle theerrfromNewFunctionRunner, and ifr.Conditionis set, return an error whenopts.CELEnvironmentis nil (or makeSetConditionreturn an error and enforce it here).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return err | ||
| } | ||
| if _, err = validator.Filter(cloneResources(selectedResources)); err != nil { | ||
| validatorRunner := validator.(*fnruntime.FunctionRunner) |
There was a problem hiding this comment.
This unchecked type assertion can panic if validator is not a *fnruntime.FunctionRunner. Prefer keeping validator typed as *fnruntime.FunctionRunner throughout, or use the “comma ok” idiom and return a descriptive error if the type is unexpected.
| validatorRunner := validator.(*fnruntime.FunctionRunner) | |
| validatorRunner, ok := validator.(*fnruntime.FunctionRunner) | |
| if !ok { | |
| err = fmt.Errorf("unexpected validator runner type %T", validator) | |
| hctx.validationSteps = append(hctx.validationSteps, preExecFailureStep(function, err)) | |
| return err | |
| } |
| ResolveToImage ImageResolveFunc | ||
|
|
||
| // CELEnvironment is the shared CEL environment used to evaluate function conditions. | ||
| // It is initialised by InitDefaults and reused across all function runners. |
There was a problem hiding this comment.
The comment says CELEnvironment is initialized by InitDefaults, but the implementation requires a separate InitCELEnvironment() call. Update the comment to match the actual initialization flow (or move initialization into InitDefaults if that’s the intended contract).
| // It is initialised by InitDefaults and reused across all function runners. | |
| // It is initialized separately via InitCELEnvironment and reused across all function runners. |
| // Initialize CEL environment for condition evaluation | ||
| // Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime | ||
| _ = r.RunnerOptions.InitCELEnvironment() |
There was a problem hiding this comment.
Ignoring the init error undermines the “proper error handling” goal described in the PR and can degrade diagnostics (users will later see “no CEL environment configured” rather than the root init failure). Since conditions can be present in Kptfiles (not just optional CLI flags), either (1) return/init-fail early here, or (2) only ignore the error when you can prove no condition will be evaluated (which isn’t knowable for fn render). Apply the same handling pattern to other callers that currently ignore the error.
|
|
||
| [SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/set-labels:latest" (condition not met) | ||
|
|
||
| Successfully executed 1 function(s) in 1 package(s). |
There was a problem hiding this comment.
In the “condition not met” example, the output says “Successfully executed 1 function(s)”, but the implementation/E2E expectations indicate skipped functions should not be counted as executed. Update the example output to reflect 0 executed functions when a step is skipped.
| Successfully executed 1 function(s) in 1 package(s). | |
| Successfully executed 0 function(s) in 1 package(s). |
| - "apply" | ||
| - "--output=json" | ||
| - "--reconcile-timeout=2m" | ||
| - "--reconcile-timeout=5m" |
There was a problem hiding this comment.
The PR description focuses on CEL-based conditional execution, but this E2E change increases reconcile timeout from 2m to 5m without being mentioned. Please either document this as part of the PR “Changes” section (with rationale), or split it into a separate PR to keep the scope aligned.
| - "live" | ||
| - "apply" | ||
| - "--reconcile-timeout=2m" | ||
| - "--reconcile-timeout=5m" |
There was a problem hiding this comment.
Same as the other live-apply fixture: this reconcile-timeout change isn’t described in the PR and appears unrelated to conditional function execution. Consider documenting the rationale in the PR or moving these timeout adjustments to a separate change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
documentation/content/en/book/04-using-functions/_index.md:1
- Two doc mismatches with the implementation: (1) the CEL resource map normalization in
resourceToMapguaranteesapiVersion,kind, andmetadatakeys, but notspec/status—either update the docs or ensure those keys are present; (2) the skipped example says “Successfully executed 1 function(s)” but the new behavior and e2e expectations indicate skipped functions should not count as executed (so this should be0).
---
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| runner, _ := fnruntime.NewFunctionRunner(r.Ctx, fltr, "", fnResult, r.fnResults, opts) | ||
| if r.Condition != "" { | ||
| runner.SetCondition(r.Condition, opts.CELEnvironment) | ||
| } | ||
| return runner, nil |
There was a problem hiding this comment.
NewFunctionRunner returns (*FunctionRunner, error) but the error is discarded. If it ever returns a non-nil error, this will return a nil runner with a nil error (and runner.SetCondition(...) can panic). Please propagate the error, and also fail fast when r.Condition != "" but opts.CELEnvironment == nil (otherwise the condition may be silently ineffective depending on runner behavior).
| func (fr *FunctionRunner) WasSkipped() bool { | ||
| return fr.skipped | ||
| } | ||
|
|
||
| func (fr *FunctionRunner) Filter(input []*yaml.RNode) (output []*yaml.RNode, err error) { | ||
| pr := printer.FromContextOrDie(fr.ctx) | ||
|
|
||
| // Check condition before executing function | ||
| if fr.celEnv != nil && fr.condition != "" { | ||
| shouldExecute, err := fr.celEnv.EvaluateCondition(fr.ctx, fr.condition, input) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to evaluate condition for function %q: %w", fr.name, err) | ||
| } | ||
|
|
||
| if !shouldExecute { |
There was a problem hiding this comment.
fr.skipped (and fr.fnResult.Skipped) are only ever set to true and never reset in Filter(). If the same FunctionRunner instance is used for more than one Filter() call, WasSkipped() can remain true and fnResult.Skipped can leak across executions. Please reset fr.skipped and fr.fnResult.Skipped at the beginning of Filter() before evaluating the condition.
| fr.fnResult.ExitCode = 0 | ||
| fr.fnResult.Skipped = true | ||
| fr.fnResults.Items = append(fr.fnResults.Items, *fr.fnResult) | ||
| // Return input unchanged - function is skipped | ||
| fr.skipped = true | ||
| return input, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
fr.skipped (and fr.fnResult.Skipped) are only ever set to true and never reset in Filter(). If the same FunctionRunner instance is used for more than one Filter() call, WasSkipped() can remain true and fnResult.Skipped can leak across executions. Please reset fr.skipped and fr.fnResult.Skipped at the beginning of Filter() before evaluating the condition.
| this function step (after `selectors` and `exclude` have been applied). Each resource is a map with | ||
| the standard fields: `apiVersion`, `kind`, `metadata`, `spec`, `status`. |
There was a problem hiding this comment.
Two doc mismatches with the implementation: (1) the CEL resource map normalization in resourceToMap guarantees apiVersion, kind, and metadata keys, but not spec/status—either update the docs or ensure those keys are present; (2) the skipped example says “Successfully executed 1 function(s)” but the new behavior and e2e expectations indicate skipped functions should not count as executed (so this should be 0).
| this function step (after `selectors` and `exclude` have been applied). Each resource is a map with | |
| the standard fields: `apiVersion`, `kind`, `metadata`, `spec`, `status`. | |
| this function step (after `selectors` and `exclude` have been applied). Each resource is normalized | |
| to include the standard fields `apiVersion`, `kind`, and `metadata`. Other fields, such as `spec` | |
| and `status`, are available when they exist on the resource. |
|
The tests are failing because we have merged the conditions and renederstatus to kpt recently. I thought that by deleting the The following `diff.patch files work for me on the tests locally on my machine: e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch Can you try restoring the two Sorry for mucking with your PR 😢 |
abd89dd to
df1189f
Compare
|
Thank you @liamfallon! I've restored both diff.patch files with exactly the contents you provided in commit df1189f. The tests should pass now. Sorry for the confusion! |
df1189f to
ad63ed9
Compare
ad63ed9 to
298cc5d
Compare
Adds support for CEL expressions in Kptfile pipeline functions via a new 'condition' field. Functions with a condition are only executed if the CEL expression evaluates to true against the current resource list. - Add CELEnvironment in pkg/lib/runneroptions/celenv.go - Integrate condition check in FunctionRunner.Filter (runner.go) - Append skipped result to fnResults when condition is not met - Add 'condition' field to kptfile/v1 Function type - Update all callers of InitDefaults to also call InitCELEnvironment - Add e2e testdata for condition-met and condition-not-met cases - Add unit tests for CEL evaluation and condition checking - Update documentation: kptfile schema and book/04-using-functions - Fix go.mod: mark k8s.io/apiserver as direct dependency Resolves kptdev#4388 Signed-off-by: SurbhiAgarwal1 <agarwalsurbhi1807@gmail.com>
298cc5d to
6d4aed1
Compare
Description
This PR adds CEL-based conditional function execution to kpt, implementing #4388.
A new optional
conditionfield is added to theFunctiontype in the Kptfile pipeline. When specified, the CEL expression is evaluated against the current list of KRM resources. If the expression returnsfalse, the function is skipped. If omitted or returnstrue, the function executes normally.Motivation
In many real-world scenarios, you want a pipeline function to run only when certain resources exist or meet specific criteria. Without this feature, users had to maintain separate Kptfiles or manually manage which functions run. The
conditionfield makes pipelines more dynamic and reduces the need for workarounds.Changes
conditionfield toFunctiontype inpkg/api/kptfile/v1/types.goCELEnvironmentinpkg/lib/runneroptions/celenv.gousinggoogle/cel-goFunctionRunner.Filter()ininternal/fnruntime/runner.go[SKIPPED]in CLI outputInitCELEnvironment()method toRunnerOptionsfor proper error handlingInitDefaults()to also callInitCELEnvironment()condition-metandcondition-not-metcasesExample Usage