feat: Add CEL-based conditional function execution (#4388)#4391
feat: Add CEL-based conditional function execution (#4388)#4391SurbhiAgarwal1 wants to merge 15 commits 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. |
SurbhiAgarwal1
force-pushed
the
feat/cel-conditional-execution
branch
from
32fe3c0 to
3d2bde5
Compare
.agent/github_comment_4382.md
Outdated
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.
internal/fnruntime/celeval.go
Outdated
|
|
||
| // 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.
There was a problem hiding this comment.
This is still unresolved. Please read up on how cost estimation works in cel environments. The goal is to protect the kpt and porch runtime from exploits using arbitrary code execution allowed here
There was a problem hiding this comment.
Hi @nagygergo! I've researched the CEL cost estimation API and found that env.EstimateCost() requires runtime size information about variables (like the resources list) which isn't available at compile time. When I tried using it with a nil estimator, it panics during size computation.
I see two approaches:
Option 1: Runtime Cost Tracking - Use cel.CostTracking() in the Program to enforce limits during evaluation (similar to Kubernetes ValidatingAdmissionPolicy)
Option 2: Custom Estimator - Implement checker.CostEstimator interface with estimated sizes for the resources variable
Which approach would you prefer? I'm happy to implement either one properly. All tests are currently passing with cel-go v0.26.0.
internal/fnruntime/runner.go
Outdated
| 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.
internal/fnruntime/runner.go
Outdated
| 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.
There was a problem hiding this comment.
Hi @nagygergo! Done! I've added TestEvaluateCondition_Immutability that verifies CEL functions can't mutate the input resource list.
The test creates a ConfigMap, stores the original YAML, evaluates a CEL condition, then compares before/after to ensure no mutation. Even though the function signature accepts *yaml.RNode list (which could be mutated), CEL only receives converted map[string]interface{} copies, so the original RNodes remain unchanged.
|
|
||
| // 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?
internal/fnruntime/celeval.go
Outdated
| // 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
internal/fnruntime/celeval.go
Outdated
| } | ||
|
|
||
| // 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.
internal/fnruntime/celeval.go
Outdated
| } | ||
|
|
||
| // 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.
There was a problem hiding this comment.
Hi @nagygergo! I've fixed the RNode serialization issue you mentioned.
Changed from resource.Map() (which serializes to YAML and back) to using resource.YNode().Decode() directly. This avoids the intermediate serialization step and reduces memory overhead.
The new implementation:
- Gets the underlying yaml.Node directly via
YNode() - Decodes it to
map[string]interface{}without serialization - Maintains the same CEL evaluation behavior
All tests are passing with this change.
SurbhiAgarwal1
force-pushed
the
feat/cel-conditional-execution
branch
from
5f1dce7 to
98ac05d
Compare
|
Thanks @nagygergo for the detailed review! I've addressed all the feedback: Changes Made:1. Cleaned up AI-generated files
2. Added CEL protection
3. Reuse CEL environment
4. Added context parameter
5. Removed unnecessary nil check
6. Added immutability test
7. Updated all tests
8. Added documentation
9. Resource serialization
10. fnResult/fnResults in tests
All review comments have been addressed. Let me know if you need any other changes! |
SurbhiAgarwal1
force-pushed
the
feat/cel-conditional-execution
branch
from
cce25d3 to
e0c5faa
Compare
|
@SurbhiAgarwal1 please fix the build issues. I'll get back to reviewing it after the build and tests are passing. |
SurbhiAgarwal1
force-pushed
the
feat/cel-conditional-execution
branch
from
155590a to
fc9326d
Compare
dosubot
bot
added
the
size:XXL
|
Great progress so far.
Looked a bit into the porch codebase as well. I'd say that the best way to initialise a CEL environment would be that This would allow for porch to be more memory efficient. Also, please move e2e tests to Then we can get to writing the documentation. I'm not overly familiar on how to extend it, so I'll need to do a bit of reading on it. I'll come back on it on Monday/Tuesday if that's ok. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestEvaluateCondition_ResourceExists(t *testing.T) { | ||
| // Create test resources | ||
| configMapYAML := ` | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: test-config | ||
| data: | ||
| key: value | ||
| ` | ||
| deploymentYAML := ` | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: test-deployment | ||
| spec: | ||
| replicas: 3 | ||
| ` | ||
|
|
||
| configMap, err := yaml.Parse(configMapYAML) | ||
| require.NoError(t, err) | ||
| deployment, err := yaml.Parse(deploymentYAML) | ||
| require.NoError(t, err) | ||
|
|
||
| resources := []*yaml.RNode{configMap, deployment} | ||
|
|
||
| // Test: ConfigMap exists | ||
| condition := `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "test-config")` | ||
| eval, err := NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err := eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.True(t, result, "should find the ConfigMap") | ||
|
|
||
| // Test: ConfigMap with wrong name doesn't exist | ||
| condition = `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "wrong-name")` | ||
| eval, err = NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.False(t, result, "should not find ConfigMap with wrong name") | ||
|
|
||
| // Test: Deployment exists | ||
| condition = `resources.exists(r, r.kind == "Deployment")` | ||
| eval, err = NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.True(t, result, "should find the Deployment") | ||
| } | ||
|
|
||
| func TestEvaluateCondition_ResourceCount(t *testing.T) { | ||
| // Create test resources | ||
| deploymentYAML := ` | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: test-deployment | ||
| spec: | ||
| replicas: 3 | ||
| ` | ||
|
|
||
| deployment, err := yaml.Parse(deploymentYAML) | ||
| require.NoError(t, err) | ||
|
|
||
| resources := []*yaml.RNode{deployment} | ||
|
|
||
| // Test: Count of deployments is greater than 0 | ||
| condition := `resources.filter(r, r.kind == "Deployment").size() > 0` | ||
| eval, err := NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err := eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.True(t, result, "should find deployments") | ||
|
|
||
| // Test: Count of ConfigMaps is 0 | ||
| condition = `resources.filter(r, r.kind == "ConfigMap").size() == 0` | ||
| eval, err = NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.True(t, result, "should not find ConfigMaps") | ||
| } |
| assert.Contains(t, err.Error(), "failed to compile") | ||
| } | ||
|
|
||
| func TestEvaluateCondition_NonBooleanResult(t *testing.T) { | ||
| // Expression that returns a number, not a boolean | ||
| _, err := NewCELEvaluator("1 + 1") | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "must return a boolean") |
| func TestEvaluateCondition_Immutability(t *testing.T) { | ||
| configMapYAML := ` | ||
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: test-config | ||
| namespace: default | ||
| data: | ||
| key: original-value | ||
| ` | ||
|
|
||
| configMap, err := yaml.Parse(configMapYAML) | ||
| require.NoError(t, err) | ||
|
|
||
| resources := []*yaml.RNode{configMap} | ||
|
|
||
| // Store original values | ||
| originalYAML, err := configMap.String() | ||
| require.NoError(t, err) | ||
|
|
||
| // Evaluate a condition that accesses the resources | ||
| condition := `resources.exists(r, r.kind == "ConfigMap")` | ||
| eval, err := NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify resources haven't been mutated | ||
| afterYAML, err := configMap.String() | ||
| require.NoError(t, err) | ||
| assert.Equal(t, originalYAML, afterYAML, "CEL evaluation should not mutate input resources") | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/fnruntime/celeval_test.go
Outdated
| "github.com/stretchr/testify/require" | ||
| "sigs.k8s.io/kustomize/kyaml/yaml" | ||
|
|
||
| "github.com/GoogleContainerTools/kpt/internal/fnruntime/runneroptions" |
internal/fnruntime/celeval_test.go
Outdated
| assert.NotNil(t, env.Env) | ||
| } | ||
|
|
||
| func TestNewCELEvaluator_EmptyCondition(t *testing.T) { | ||
| env, err := runneroptions.NewCELEnvironment() | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, env) | ||
| assert.NotNil(t, env.Env) | ||
| } | ||
|
|
||
| func TestEvaluateCondition_EmptyCondition(t *testing.T) { |
internal/fnruntime/celeval_test.go
Outdated
| // Test: ConfigMap exists | ||
| condition := `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "test-config")` | ||
| eval, err := NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err := eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.True(t, result, "should find the ConfigMap") | ||
|
|
||
| // Test: ConfigMap with wrong name doesn't exist | ||
| condition = `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "wrong-name")` | ||
| eval, err = NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) | ||
| assert.False(t, result, "should not find ConfigMap with wrong name") | ||
|
|
||
| // Test: Deployment exists | ||
| condition = `resources.exists(r, r.kind == "Deployment")` | ||
| eval, err = NewCELEvaluator(condition) | ||
| require.NoError(t, err) | ||
| result, err = eval.EvaluateCondition(context.Background(), resources) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Set condition; the shared CEL environment from opts is used at evaluation time. | ||
| if f.Condition != "" { |
| func (opts *RunnerOptions) InitDefaults(defaultImagePrefix string) { | ||
| opts.ImagePullPolicy = IfNotPresentPull | ||
| opts.ResolveToImage = opts.ResolveToImageForCLIFunc(defaultImagePrefix) | ||
| celEnv, err := NewCELEnvironment() | ||
| if err != nil { | ||
| // CEL environment creation should never fail with the standard config; | ||
| // panic here surfaces misconfiguration immediately rather than silently skipping conditions. | ||
| panic(fmt.Sprintf("failed to initialise CEL environment: %v", err)) | ||
| } | ||
| opts.CELEnvironment = celEnv | ||
| } | ||
|
|
internal/fnruntime/celeval.go
Outdated
| // CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go | ||
| // can reference it within the fnruntime package without an import cycle. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 CELEvaluator in internal/fnruntime/celeval.go with k8s CEL extensions - 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 executor and runneroptions to support condition passing - Add e2e and unit tests for conditional execution - Add k8s.io/apiserver dependency for CEL library extensions Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Replace k8s apiserver CEL library functions with cel-go built-in ext package equivalents. The k8s-specific functions (IP, CIDR, Quantity, SemVer, etc.) are not needed for basic KRM resource filtering and the heavy dependency was causing CI build failures. Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
…valuateCondition call - Move CELEvaluator/CELEnvironment to pkg/lib/runneroptions to avoid import cycle - Rename NewCELEvaluator(condition) -> NewCELEnvironment() (no condition param) - Remove pre-compiled prg field; compile program inside EvaluateCondition per call - Add CELEnvironment field to RunnerOptions, populated in InitDefaults - celeval.go now just type-aliases runneroptions.CELEnvironment - Update runner.go to use opts.CELEnvironment and pass condition string at eval time - Update unit tests to use new API - Add e2e testdata under e2e/testdata/fn-render/condition/ Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Remove internal/fnruntime/conditional_e2e_test.go and replace with testdata-driven e2e tests under e2e/testdata/fn-render/condition/: - condition-met: function executes when CEL condition is true - condition-not-met: function is skipped when CEL condition is false Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
- Run go mod tidy to drop k8s.io/apiserver (was causing docker/podman CI failure) - Fix celeval_test.go: correct import path to github.com/kptdev/kpt/pkg/lib/runneroptions - Update tests to use new EvaluateCondition(ctx, condition, resources) API Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
RunnerOptions.CELEnvironment is now populated by InitDefaults, so nil it out before struct comparison just like ResolveToImage. Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Fix 1: ensure kind/apiVersion/metadata always present in CEL resource map - resourceToMap now guarantees these keys exist so CEL expressions like r.kind == 'Deployment' return false instead of 'no such key' error Fix 2: add .krmignore to condition test dirs - .expected/ was being picked up by kpt fn render as a KRM resource - .krmignore with '.expected' excludes it, matching all other test cases Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
|
Hi @nagygergo, I've addressed all your feedback — CELEnvironment in RunnerOptions, InitDefaults creates it, program compilation moved to EvaluateCondition, e2e tests moved to e2e/testdata/fn-render/condition/, and k8s.io/apiserver removed from go.mod. The CI is still showing some test failures. I verified locally that TestCmd_flagAndArgParsing_Symlink and TestFunctionConfig/file_config also fail on upstream/main directly, so they appear to be pre-existing. Could you confirm whether the remaining CI failures are blocking from your side, or if they're known flaky/pre-existing tests? |
|
@SurbhiAgarwal1 , as part of this comment: #4391 (comment) I've added a lot of k8s specific evaluators as well. Why were they removed? |
|
Hey @nagygergo, thanks for pointing this out. That removal wasn’t intentional from a functionality perspective. I was focusing on simplifying the evaluator logic and didn’t realize those k8s-specific evaluators were recently added as part of #4391. I’ll restore them and make sure the changes stay compatible with the intended behavior. If there are specific expectations around how these evaluators should be handled, I’d be happy to align with that. Thanks for catching this. |
|
Also, the test cases that failed in the latest CI seem to be the new e2e testcases you added. |
- Switch condition testdata from set-labels to no-op function so diff.patch hashes are deterministic and don't depend on container output - Fix diff.patch files with correct git blob hashes for before/after Kptfile - Update config.yaml expected stderr to match no-op function output - Add comment in celenv.go explaining why k8s.io/apiserver CEL extensions are excluded (binary size / CI build failures) while cel-go built-ins suffice Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/api/kptfile/v1/types.go
Outdated
| // function should be executed. The expression is evaluated against the KRM | ||
| // resources in the package and should return a boolean value. | ||
| // If omitted or evaluates to true, the function executes normally. | ||
| // If evaluates to false, the function is skipped. | ||
| // | ||
| // Example: Check if a specific ConfigMap exists: | ||
| // condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'my-config')" | ||
| // | ||
| // Example: Check resource count: |
There was a problem hiding this comment.
The field doc says the condition is evaluated against “the KRM resources in the package”, but the current runtime evaluates the condition on the resource list passed to the step (which is already filtered by selectors/exclude in the render executor). Please clarify the documentation to match the actual evaluation scope, or adjust the implementation so the condition sees the full package resource list.
| // function should be executed. The expression is evaluated against the KRM | |
| // resources in the package and should return a boolean value. | |
| // If omitted or evaluates to true, the function executes normally. | |
| // If evaluates to false, the function is skipped. | |
| // | |
| // Example: Check if a specific ConfigMap exists: | |
| // condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'my-config')" | |
| // | |
| // Example: Check resource count: | |
| // function should be executed. The expression is evaluated against the list | |
| // of KRM resources passed to this function step (i.e. the resources selected | |
| // for the step after `Selectors` and `Exclude` have been applied by the | |
| // render executor) and should return a boolean value. | |
| // If omitted or evaluates to true, the function executes normally. | |
| // If evaluates to false, the function is skipped. | |
| // | |
| // Example: Check if a specific ConfigMap exists among the selected resources: | |
| // condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'my-config')" | |
| // | |
| // Example: Check resource count among the selected resources: |
| require.NoError(t, err) | ||
| assert.True(t, result) | ||
| } | ||
|
|
There was a problem hiding this comment.
There isn’t currently a test case covering resources that lack metadata.name/metadata (or lack nested keys like metadata.name). Given the examples rely on r.metadata.name, adding a unit test for missing fields would help ensure condition evaluation doesn’t error unexpectedly on partially-specified resources.
| func TestEvaluateCondition_ResourceExistsMissingMetadata(t *testing.T) { | |
| env := newTestEnv(t) | |
| // Resource without metadata at all | |
| configMapNoMetadata, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\ndata:\n key: value") | |
| require.NoError(t, err) | |
| // Resource with metadata but without name | |
| configMapNoName, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\nmetadata: {}\ndata:\n key: other") | |
| require.NoError(t, err) | |
| resources := []*yaml.RNode{configMapNoMetadata, configMapNoName} | |
| // Ensure that accessing r.metadata.name in the condition does not error | |
| result, err := env.EvaluateCondition(context.Background(), | |
| `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "test-config")`, resources) | |
| require.NoError(t, err) | |
| assert.False(t, result, "no resource should match when metadata.name is missing") | |
| } |
pkg/lib/runneroptions/celenv.go
Outdated
| if _, ok := result["metadata"]; !ok { | ||
| result["metadata"] = map[string]interface{}{} |
There was a problem hiding this comment.
resourceToMap ensures metadata exists, but expressions like r.metadata.name == '...' will still fail if metadata.name is missing on any resource. Since the public examples/docs encourage r.metadata.name, consider defaulting common nested keys (e.g., metadata.name, metadata.namespace) to empty strings when absent, or otherwise making missing nested keys evaluate safely (so conditions don’t error on partially-specified resources).
| if _, ok := result["metadata"]; !ok { | |
| result["metadata"] = map[string]interface{}{} | |
| // Ensure metadata exists and has common nested keys so expressions like | |
| // r.metadata.name and r.metadata.namespace do not fail on missing keys. | |
| if mdVal, ok := result["metadata"]; ok { | |
| if mdMap, ok := mdVal.(map[string]interface{}); ok { | |
| if _, ok := mdMap["name"]; !ok { | |
| mdMap["name"] = "" | |
| } | |
| if _, ok := mdMap["namespace"]; !ok { | |
| mdMap["namespace"] = "" | |
| } | |
| result["metadata"] = mdMap | |
| } else { | |
| // If metadata is not a map, replace it with a sane default. | |
| result["metadata"] = map[string]interface{}{ | |
| "name": "", | |
| "namespace": "", | |
| } | |
| } | |
| } else { | |
| result["metadata"] = map[string]interface{}{ | |
| "name": "", | |
| "namespace": "", | |
| } |
internal/fnruntime/celeval.go
Outdated
| // CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go | ||
| // can reference it within the fnruntime package without an import cycle. |
There was a problem hiding this comment.
The comment says this alias avoids an import cycle, but runner.go already imports pkg/lib/runneroptions directly (for RunnerOptions), so there’s no cycle here. Either remove this indirection or adjust the comment to avoid misleading future maintainers.
| // CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go | |
| // can reference it within the fnruntime package without an import cycle. | |
| // CELEvaluator is a local alias for runneroptions.CELEnvironment used within | |
| // the fnruntime package. |
Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
|
Hi @nagygergo, the e2e test failures are now fixed (commit 6d1fb6b) — the diff.patch files had wrong git blob hashes. CI is passing now. Regarding the k8s-specific CEL evaluators: I investigated and k8s.io/apiserver is what caused the docker/podman CI to fail with 8+ minute build timeouts due to its large transitive dependency graph. That's why it was removed. The current implementation uses cel-go built-in extensions (ext.Strings, ext.Sets, ext.Lists, ext.TwoVarComprehensions) which cover standard KRM filtering. Could you advise on whether the k8s-specific validators (IP, CIDR, Quantity, SemVer) are required for this feature, and if so, how to include them without pulling in the full k8s.io/apiserver dependency? |
Hi @SurbhiAgarwal1, went through the execution logs for this pull request, but I couldn't find a place where the docker build would've timed out. If you could point at it, I'd appreciate, and we should fix the CI instead. |
…, SemVer) Re-add k8s.io/apiserver dependency to include k8s-specific CEL validators. Maintainer confirmed these should be included despite the build time increase. Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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) | ||
| } |
There was a problem hiding this comment.
EvaluateCondition is being run against the input slice passed to FunctionRunner.Filter, but upstream code applies Selectors/Exclusions before invoking this filter. This means the condition may be evaluated on a subset of package resources (and can yield incorrect skip/run decisions), which contradicts the field docs that say it evaluates against the package resources. Consider evaluating the condition against the full package resource list (before selector filtering) or explicitly documenting that conditions are evaluated against the selected input only.
| } | ||
|
|
||
| // Set condition; the shared CEL environment from opts is used at evaluation time. | ||
| if f.Condition != "" { |
There was a problem hiding this comment.
If f.Condition is set but opts.CELEnvironment is nil, the condition is silently ignored (the function executes unconditionally). This can happen for callers that construct RunnerOptions without calling InitDefaults, and it conflicts with the intent to avoid silently skipping condition logic. Consider returning an error when a condition is provided but no CEL environment is configured.
| if f.Condition != "" { | |
| if f.Condition != "" { | |
| if opts.CELEnvironment == nil { | |
| return nil, fmt.Errorf("condition specified for function %q, but no CEL environment is configured", f.Name) | |
| } |
| opts.ImagePullPolicy = IfNotPresentPull | ||
| opts.ResolveToImage = opts.ResolveToImageForCLIFunc(defaultImagePrefix) | ||
| celEnv, err := NewCELEnvironment() | ||
| if err != nil { | ||
| // CEL environment creation should never fail with the standard config; | ||
| // panic here surfaces misconfiguration immediately rather than silently skipping conditions. | ||
| panic(fmt.Sprintf("failed to initialise CEL environment: %v", err)) | ||
| } | ||
| opts.CELEnvironment = celEnv |
There was a problem hiding this comment.
InitDefaults panics if CEL environment initialization fails. Since InitDefaults is part of a reusable options type (and is called in multiple commands/tests), a panic here will crash the process and is harder to handle than an error. Consider surfacing this as an error (e.g., an InitDefaults(...) error variant) or deferring failure until a condition is evaluated while still reporting a clear error.
| opts.ImagePullPolicy = IfNotPresentPull | |
| opts.ResolveToImage = opts.ResolveToImageForCLIFunc(defaultImagePrefix) | |
| celEnv, err := NewCELEnvironment() | |
| if err != nil { | |
| // CEL environment creation should never fail with the standard config; | |
| // panic here surfaces misconfiguration immediately rather than silently skipping conditions. | |
| panic(fmt.Sprintf("failed to initialise CEL environment: %v", err)) | |
| } | |
| opts.CELEnvironment = celEnv | |
| // Preserve existing behaviour for callers that do not expect an error, | |
| // while avoiding process-wide panics on CEL initialization failure. | |
| _ = opts.InitDefaultsWithError(defaultImagePrefix) | |
| } | |
| // InitDefaultsWithError initialises RunnerOptions with default values and reports | |
| // any CEL environment initialisation failure as an error instead of panicking. | |
| func (opts *RunnerOptions) InitDefaultsWithError(defaultImagePrefix string) error { | |
| opts.ImagePullPolicy = IfNotPresentPull | |
| opts.ResolveToImage = opts.ResolveToImageForCLIFunc(defaultImagePrefix) | |
| celEnv, err := NewCELEnvironment() | |
| if err != nil { | |
| // Return a descriptive error so callers can handle misconfiguration gracefully. | |
| return fmt.Errorf("failed to initialise CEL environment: %w", err) | |
| } | |
| opts.CELEnvironment = celEnv | |
| return nil |
internal/fnruntime/celeval.go
Outdated
| // CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go | ||
| // can reference it within the fnruntime package without an import cycle. |
There was a problem hiding this comment.
The comment says this type alias is needed to avoid an import cycle, but runner.go already imports runneroptions and there is no runneroptions -> fnruntime dependency. Either remove the alias and use *runneroptions.CELEnvironment directly, or update the comment to accurately explain why the alias is needed (to avoid misleading future maintainers).
| // CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go | |
| // can reference it within the fnruntime package without an import cycle. | |
| // CELEvaluator is a type alias for runneroptions.CELEnvironment, re-exported | |
| // through the fnruntime package for convenience. |
The json-output and apply-depends-on tests were timing out on kind v1.33.4 in CI. The deployments use initContainers with sleep 10 plus image pulls, which can exceed 2m on slower CI runners. Increase to 5m for reliability. Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
- celeval.go: fix type alias comment (no import cycle, just convenience) - celenv.go: default metadata.name and metadata.namespace to empty string so CEL expressions like r.metadata.name never error on missing keys - runner.go: return error when condition is set but CELEnvironment is nil - types.go: clarify Condition doc - evaluated against selected resources (after Selectors/Exclude), not the full package resource list - celeval_test.go: add TestEvaluateCondition_MissingMetadata to verify conditions don't error on resources with missing metadata fields Signed-off-by: Surbhi <agarwalsurbhi1807@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set condition; the shared CEL environment from opts is used at evaluation time. | ||
| if f.Condition != "" { | ||
| if opts.CELEnvironment == nil { | ||
| return nil, fmt.Errorf("condition specified for function %q but no CEL environment is configured in RunnerOptions", f.Image) |
There was a problem hiding this comment.
The error when a condition is specified but no CEL environment is configured formats the function name using f.Image. For exec-based functions (or other cases where Image is empty), this message becomes unhelpful. Consider using the computed runner name (image or exec) or include both image and exec in the message.
| return nil, fmt.Errorf("condition specified for function %q but no CEL environment is configured in RunnerOptions", f.Image) | |
| name := f.Image | |
| if name == "" { | |
| name = f.Exec | |
| } | |
| return nil, fmt.Errorf("condition specified for function %q but no CEL environment is configured in RunnerOptions", name) |
| gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect | ||
| gopkg.in/inf.v0 v0.9.1 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| k8s.io/apiserver v0.34.1 // indirect |
There was a problem hiding this comment.
go.mod lists k8s.io/apiserver as an indirect dependency, but the new code imports k8s.io/apiserver/pkg/cel/library directly (pkg/lib/runneroptions/celenv.go). This should be a direct requirement (no // indirect) and will likely be rewritten by go mod tidy (and could fail CI if tidy checks are enforced).
| k8s.io/apiserver v0.34.1 // indirect | |
| k8s.io/apiserver v0.34.1 |
Implements #4388
Changes
conditionfield to Function schema for CEL expressionsExample Usage