From aec1d1e898522fdf3ba959bb98fde180f214b176 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Tue, 17 Mar 2026 10:13:00 +0530 Subject: [PATCH 01/19] feat: Add CEL-based conditional function execution 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 --- go.mod | 8 + go.sum | 21 ++ internal/fnruntime/celeval.go | 166 ++++++++++++ internal/fnruntime/celeval_test.go | 200 ++++++++++++++ internal/fnruntime/conditional_e2e_test.go | 286 +++++++++++++++++++++ internal/fnruntime/runner.go | 44 +++- pkg/api/kptfile/v1/types.go | 13 + 7 files changed, 737 insertions(+), 1 deletion(-) create mode 100644 internal/fnruntime/celeval.go create mode 100644 internal/fnruntime/celeval_test.go create mode 100644 internal/fnruntime/conditional_e2e_test.go diff --git a/go.mod b/go.mod index 40a6bd74d7..0b467aaaf1 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/bytecodealliance/wasmtime-go v1.0.0 github.com/cpuguy83/go-md2man/v2 v2.0.7 github.com/go-errors/errors v1.5.1 + github.com/google/cel-go v0.26.0 github.com/google/go-cmp v0.7.0 github.com/google/go-containerregistry v0.20.6 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 @@ -28,6 +29,7 @@ require ( k8s.io/api v0.34.1 k8s.io/apiextensions-apiserver v0.34.1 k8s.io/apimachinery v0.34.1 + k8s.io/apiserver v0.34.1 k8s.io/cli-runtime v0.34.1 k8s.io/client-go v0.34.1 k8s.io/component-base v0.34.1 @@ -42,9 +44,11 @@ require ( ) require ( + cel.dev/expr v0.24.0 // indirect cloud.google.com/go/compute/metadata v0.9.0 // indirect github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c // indirect github.com/MakeNowJust/heredoc v1.0.0 // indirect + github.com/antlr4-go/antlr/v4 v4.13.0 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/blang/semver/v4 v4.0.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect @@ -109,6 +113,7 @@ require ( github.com/sergi/go-diff v1.4.0 // indirect github.com/sirupsen/logrus v1.9.3 // indirect github.com/spyzhov/ajson v0.9.6 // indirect + github.com/stoewer/go-strcase v1.3.0 // indirect github.com/ulikunitz/xz v0.5.15 // indirect github.com/vbatts/tar-split v0.12.2 // indirect github.com/x448/float16 v0.8.4 // indirect @@ -116,12 +121,15 @@ require ( go.opentelemetry.io/otel/trace v1.38.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect + golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect golang.org/x/net v0.47.0 // indirect golang.org/x/oauth2 v0.32.0 // indirect golang.org/x/sync v0.18.0 // indirect golang.org/x/sys v0.38.0 // indirect golang.org/x/term v0.37.0 // indirect golang.org/x/time v0.14.0 // indirect + google.golang.org/genproto/googleapis/api v0.0.0-20250303144028-a0af3efb3deb // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20250303144028-a0af3efb3deb // indirect google.golang.org/protobuf v1.36.10 // indirect gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect diff --git a/go.sum b/go.sum index 0d88e5c71d..fe538b32d5 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +cel.dev/expr v0.24.0 h1:56OvJKSH3hDGL0ml5uSxZmz3/3Pq4tJ+fb1unVLAFcY= +cel.dev/expr v0.24.0/go.mod h1:hLPLo1W4QUmuYdA72RBX06QTs6MXw941piREPl3Yfiw= cloud.google.com/go/compute/metadata v0.9.0 h1:pDUj4QMoPejqq20dK0Pg2N4yG9zIkYGdBtwLoEkH9Zs= cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10= github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c h1:udKWzYgxTojEKWjV8V+WSxDXJ4NFATAsZjh8iIbsQIg= @@ -6,6 +8,8 @@ github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ github.com/MakeNowJust/heredoc v1.0.0/go.mod h1:mG5amYoWBHf8vpLOuehzbGGw0EHxpZZ6lCpQ4fNJ8LE= github.com/Masterminds/semver/v3 v3.4.0 h1:Zog+i5UMtVoCU8oKka5P7i9q9HgrJeGzI9SA1Xbatp0= github.com/Masterminds/semver/v3 v3.4.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= +github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI= +github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= @@ -89,6 +93,8 @@ github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/google/btree v1.1.3 h1:CVpQJjYgC4VbzxeGVHfvZrv1ctoYCAI8vbl07Fcxlyg= github.com/google/btree v1.1.3/go.mod h1:qOPhT0dTNdNzV6Z/lhRX0YXUafgPLFUh+gZMl761Gm4= +github.com/google/cel-go v0.26.0 h1:DPGjXackMpJWH680oGY4lZhYjIameYmR+/6RBdDGmaI= +github.com/google/cel-go v0.26.0/go.mod h1:A9O8OU9rdvrK5MQyrqfIxo1a0u4g3sF8KB6PUIaryMM= github.com/google/gnostic-models v0.7.0 h1:qwTtogB15McXDaNqTZdzPJRHvaVJlAl+HVQnLmJEJxo= github.com/google/gnostic-models v0.7.0/go.mod h1:whL5G0m6dmc5cPxKc5bdKdEN3UjI7OUGxBlw57miDrQ= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= @@ -203,13 +209,20 @@ github.com/spf13/pflag v1.0.10 h1:4EBh2KAYBwaONj6b2Ye1GiHfwjqyROoF4RwYO+vPwFk= github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spyzhov/ajson v0.9.6 h1:iJRDaLa+GjhCDAt1yFtU/LKMtLtsNVKkxqlpvrHHlpQ= github.com/spyzhov/ajson v0.9.6/go.mod h1:a6oSw0MMb7Z5aD2tPoPO+jq11ETKgXUr2XktHdT8Wt8= +github.com/stoewer/go-strcase v1.3.0 h1:g0eASXYtp+yvN9fK8sH94oCIk0fau9uV1/ZdJ0AVEzs= +github.com/stoewer/go-strcase v1.3.0/go.mod h1:fAH5hQ5pehh+j3nZfvwdk2RgEgQjAoM8wodgtPmh1xo= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/ulikunitz/xz v0.5.15 h1:9DNdB5s+SgV3bQ2ApL10xRc35ck0DuIX/isZvIk+ubY= @@ -239,6 +252,8 @@ go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.29.0 h1:HV8lRxZC4l2cr3Zq1LvtOsi/ThTgWnUk/y64QSs8GwA= @@ -281,6 +296,10 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +google.golang.org/genproto/googleapis/api v0.0.0-20250303144028-a0af3efb3deb h1:p31xT4yrYrSM/G4Sn2+TNUkVhFCbG9y8itM2S6Th950= +google.golang.org/genproto/googleapis/api v0.0.0-20250303144028-a0af3efb3deb/go.mod h1:jbe3Bkdp+Dh2IrslsFCklNhweNTBgSYanP1UXhJDhKg= +google.golang.org/genproto/googleapis/rpc v0.0.0-20250303144028-a0af3efb3deb h1:TLPQVbx1GJ8VKZxz52VAxl1EBgKXXbTiU9Fc5fZeLn4= +google.golang.org/genproto/googleapis/rpc v0.0.0-20250303144028-a0af3efb3deb/go.mod h1:LuRYeWDFV6WOn90g357N17oMCaxpgCnbi/44qJvDn2I= google.golang.org/protobuf v1.36.10 h1:AYd7cD/uASjIL6Q9LiTjz8JLcrh/88q5UObnmY3aOOE= google.golang.org/protobuf v1.36.10/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= @@ -307,6 +326,8 @@ k8s.io/apiextensions-apiserver v0.34.1 h1:NNPBva8FNAPt1iSVwIE0FsdrVriRXMsaWFMqJb k8s.io/apiextensions-apiserver v0.34.1/go.mod h1:hP9Rld3zF5Ay2Of3BeEpLAToP+l4s5UlxiHfqRaRcMc= k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= +k8s.io/apiserver v0.34.1 h1:U3JBGdgANK3dfFcyknWde1G6X1F4bg7PXuvlqt8lITA= +k8s.io/apiserver v0.34.1/go.mod h1:eOOc9nrVqlBI1AFCvVzsob0OxtPZUCPiUJL45JOTBG0= k8s.io/cli-runtime v0.34.1 h1:btlgAgTrYd4sk8vJTRG6zVtqBKt9ZMDeQZo2PIzbL7M= k8s.io/cli-runtime v0.34.1/go.mod h1:aVA65c+f0MZiMUPbseU/M9l1Wo2byeaGwUuQEQVVveE= k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY= diff --git a/internal/fnruntime/celeval.go b/internal/fnruntime/celeval.go new file mode 100644 index 0000000000..71cc9a0f8e --- /dev/null +++ b/internal/fnruntime/celeval.go @@ -0,0 +1,166 @@ +// Copyright 2026 The kpt and Nephio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fnruntime + +import ( + "context" + "fmt" + + "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/ext" + k8scellib "k8s.io/apiserver/pkg/cel/library" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +const checkFrequency = 100 + +// This gives about .1 seconds of CPU time for the evaluation to run +const costLimit = 1000000 + +// CELEvaluator evaluates CEL expressions against KRM resources +type CELEvaluator struct { + env *cel.Env + prg cel.Program // Pre-compiled program for the condition +} + +// NewCELEvaluator creates a new CEL evaluator with the standard environment +// for the given condition string. +func NewCELEvaluator(condition string) (*CELEvaluator, error) { + env, err := cel.NewEnv( + cel.Variable("resources", cel.ListType(cel.DynType)), + // Below is a list of Env settings that is a selection of https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go + // General rules are for maintaining this list. + // 1. utility functions should be available. This allows for more compatibility with k8s's own CEL conditions + // 2. AST validation is not needed as kpt will recompile CEL expressions every time, there is no cost-saving in exiting early + // 3. Compile time optimisations do not make sense, as each CEL expression will be evaluated once before being discarded. + // 3. Things that are helping with authorization in k8s are not needed, as they're returning either ResourceCheck or Decision types, which are not needed for kpt + cel.HomogeneousAggregateLiterals(), + cel.DefaultUTCTimeZone(true), + k8scellib.URLs(), + k8scellib.Regex(), + k8scellib.Lists(), + cel.CrossTypeNumericComparisons(true), + cel.OptionalTypes(), + k8scellib.Quantity(), + ext.Strings(ext.StringsVersion(2)), + ext.Sets(), + k8scellib.IP(), + k8scellib.CIDR(), + k8scellib.Format(), + ext.TwoVarComprehensions(), + k8scellib.SemverLib(k8scellib.SemverVersion(1)), + ext.Lists(ext.ListsVersion(3)), + ) + if err != nil { + return nil, fmt.Errorf("failed to create CEL environment: %w", err) + } + + evaluator := &CELEvaluator{ + env: env, + } + + // Pre-compile the condition if provided + if condition != "" { + ast, issues := env.Compile(condition) + if issues != nil && issues.Err() != nil { + return nil, fmt.Errorf("failed to compile CEL expression: %w", issues.Err()) + } + + // Validate that the expression returns a boolean + if ast.OutputType() != cel.BoolType { + return nil, fmt.Errorf("CEL expression must return a boolean, got %v", ast.OutputType()) + } + + // Create the program with a hard cost limit and cost tracking enabled + prg, err := env.Program(ast, + cel.CostLimit(costLimit), + cel.InterruptCheckFrequency(checkFrequency), + cel.CostTracking(&k8scellib.CostEstimator{}), + ) + if err != nil { + return nil, fmt.Errorf("failed to create CEL program: %w", err) + } + + evaluator.prg = prg + } + + return evaluator, nil +} + +// EvaluateCondition evaluates a CEL condition expression against a list of resources +// Returns true if the condition is met, false otherwise +// The program is pre-compiled, so this just evaluates it with the given resources +func (e *CELEvaluator) EvaluateCondition(ctx context.Context, resources []*yaml.RNode) (bool, error) { + if e.prg == nil { + return true, nil + } + + // Convert resources to a format suitable for CEL + resourceList, err := e.resourcesToList(resources) + if err != nil { + return false, fmt.Errorf("failed to convert resources: %w", err) + } + + // Evaluate the expression + out, _, err := e.prg.ContextEval(ctx, map[string]interface{}{ + "resources": resourceList, + }) + if err != nil { + return false, fmt.Errorf("failed to evaluate CEL expression: %w", err) + } + + // Extract the boolean result + result, ok := out.(types.Bool) + if !ok { + return false, fmt.Errorf("CEL expression must return a boolean, got %T", out) + } + + return bool(result), nil +} + +// resourcesToList converts RNodes to a list of maps for CEL evaluation +func (e *CELEvaluator) resourcesToList(resources []*yaml.RNode) ([]interface{}, error) { + result := make([]interface{}, 0, len(resources)) + + for _, resource := range resources { + // Convert each resource to a map + resourceMap, err := e.resourceToMap(resource) + if err != nil { + return nil, err + } + result = append(result, resourceMap) + } + + return result, nil +} + +// resourceToMap converts a single RNode to a map for CEL evaluation +// Converts yaml.Node directly to avoid serialization overhead +func (e *CELEvaluator) resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { + // Get the underlying yaml.Node + node := resource.YNode() + if node == nil { + return nil, fmt.Errorf("resource has nil yaml.Node") + } + + // Convert yaml.Node to map[string]interface{} directly + var result map[string]interface{} + if err := node.Decode(&result); err != nil { + return nil, fmt.Errorf("failed to decode resource: %w", err) + } + + return result, nil +} diff --git a/internal/fnruntime/celeval_test.go b/internal/fnruntime/celeval_test.go new file mode 100644 index 0000000000..e55c2abc1f --- /dev/null +++ b/internal/fnruntime/celeval_test.go @@ -0,0 +1,200 @@ +// Copyright 2026 The kpt and Nephio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fnruntime + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +func TestNewCELEvaluator(t *testing.T) { + eval, err := NewCELEvaluator("true") + require.NoError(t, err) + assert.NotNil(t, eval) + assert.NotNil(t, eval.env) + assert.NotNil(t, eval.prg) +} + +func TestNewCELEvaluator_EmptyCondition(t *testing.T) { + eval, err := NewCELEvaluator("") + require.NoError(t, err) + assert.NotNil(t, eval) + assert.NotNil(t, eval.env) + assert.Nil(t, eval.prg) +} + +func TestEvaluateCondition_EmptyCondition(t *testing.T) { + eval, err := NewCELEvaluator("") + require.NoError(t, err) + + result, err := eval.EvaluateCondition(context.Background(), nil) + require.NoError(t, err) + assert.True(t, result, "empty condition should return true") +} + +func TestEvaluateCondition_SimpleTrue(t *testing.T) { + eval, err := NewCELEvaluator("true") + require.NoError(t, err) + + result, err := eval.EvaluateCondition(context.Background(), nil) + require.NoError(t, err) + assert.True(t, result) +} + +func TestEvaluateCondition_SimpleFalse(t *testing.T) { + eval, err := NewCELEvaluator("false") + require.NoError(t, err) + + result, err := eval.EvaluateCondition(context.Background(), nil) + require.NoError(t, err) + assert.False(t, result) +} + +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") +} + +func TestEvaluateCondition_InvalidExpression(t *testing.T) { + // Test invalid syntax + _, err := NewCELEvaluator("this is not valid CEL") + assert.Error(t, err) + 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") +} + +// TestEvaluateCondition_Immutability ensures CEL evaluation cannot mutate the input resources +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") +} diff --git a/internal/fnruntime/conditional_e2e_test.go b/internal/fnruntime/conditional_e2e_test.go new file mode 100644 index 0000000000..f3c47d2539 --- /dev/null +++ b/internal/fnruntime/conditional_e2e_test.go @@ -0,0 +1,286 @@ +// Copyright 2026 The kpt and Nephio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fnruntime + +import ( + "context" + "io" + "testing" + + "github.com/kptdev/kpt/internal/types" + fnresultv1 "github.com/kptdev/kpt/pkg/api/fnresult/v1" + "github.com/kptdev/kpt/pkg/lib/runneroptions" + "github.com/kptdev/kpt/pkg/printer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/kustomize/kyaml/filesys" + "sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil" + "sigs.k8s.io/kustomize/kyaml/kio" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +// TestFunctionRunner_ConditionalExecution_E2E tests the complete flow +// of conditional function execution +func TestFunctionRunner_ConditionalExecution_E2E(t *testing.T) { + ctx := printer.WithContext(context.Background(), printer.New(nil, nil)) + _ = filesys.MakeFsInMemory() // Reserved for future use + + testCases := []struct { + name string + condition string + inputResources []string + shouldExecute bool + description string + }{ + { + name: "condition met - ConfigMap exists", + condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, + inputResources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config +data: + env: production`, + }, + shouldExecute: true, + description: "Function should execute when ConfigMap with specific name exists", + }, + { + name: "condition not met - ConfigMap missing", + condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, + inputResources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: other-config +data: + env: staging`, + }, + shouldExecute: false, + description: "Function should skip when specified ConfigMap doesn't exist", + }, + { + name: "condition met - Deployment count check", + condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, + inputResources: []string{ + `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web-app +spec: + replicas: 3`, + }, + shouldExecute: true, + description: "Function should execute when Deployments exist", + }, + { + name: "condition not met - no Deployments", + condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, + inputResources: []string{ + `apiVersion: v1 +kind: Service +metadata: + name: web-service`, + }, + shouldExecute: false, + description: "Function should skip when no Deployments exist", + }, + { + name: "always true condition", + condition: `true`, + inputResources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: test`, + }, + shouldExecute: true, + description: "Function should always execute with true condition", + }, + { + name: "always false condition", + condition: `false`, + inputResources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: test`, + }, + shouldExecute: false, + description: "Function should never execute with false condition", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Parse input resources + var input []*yaml.RNode + for _, resourceYAML := range tc.inputResources { + rnode, err := yaml.Parse(resourceYAML) + require.NoError(t, err) + input = append(input, rnode) + } + + // Create a mock function that adds an annotation + functionExecuted := false + mockFilter := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { + functionExecuted = true + // Add an annotation to mark execution + for _, node := range nodes { + err := node.PipeE( + yaml.SetAnnotation("test-annotation", "executed"), + ) + if err != nil { + return nil, err + } + } + return nodes, nil + } + + // Create adapter function to match FunctionFilter.Run signature + adapterFunc := func(reader io.Reader, writer io.Writer) error { + // Parse YAML from reader into RNodes + nodes, err := (&kio.ByteReader{Reader: reader}).Read() + if err != nil { + return err + } + + // Call mockFilter + resultNodes, err := mockFilter(nodes) + if err != nil { + return err + } + + // Write results back to writer + return (&kio.ByteWriter{Writer: writer}).Write(resultNodes) + } + + // Create function runner with condition + fnResult := &fnresultv1.Result{} + fnResults := &fnresultv1.ResultList{} + + evaluator, err := NewCELEvaluator(tc.condition) + require.NoError(t, err) + + runner := &FunctionRunner{ + ctx: ctx, + name: "test-function", + pkgPath: types.UniquePath("test"), + disableCLIOutput: true, + filter: &runtimeutil.FunctionFilter{ + Run: adapterFunc, + }, + fnResult: fnResult, + fnResults: fnResults, + opts: runneroptions.RunnerOptions{}, + condition: tc.condition, + evaluator: evaluator, + } + + // Execute the filter + output, err := runner.Filter(input) + require.NoError(t, err) + + // Verify function execution based on condition + if tc.shouldExecute { + assert.True(t, functionExecuted, tc.description) + // Verify annotation was added + annotations := output[0].GetAnnotations() + annotation := annotations["test-annotation"] + assert.Equal(t, "executed", annotation) + } else { + assert.False(t, functionExecuted, tc.description) + // Verify output is unchanged (no annotation) + annotations := output[0].GetAnnotations() + _, exists := annotations["test-annotation"] + assert.False(t, exists, "annotation should not exist when function is skipped") + } + }) + } +} + +// TestFunctionRunner_ConditionalExecution_ComplexConditions tests more advanced CEL expressions +func TestFunctionRunner_ConditionalExecution_ComplexConditions(t *testing.T) { + ctx := context.Background() + + testCases := []struct { + name string + condition string + resources []string + shouldExecute bool + }{ + { + name: "multiple conditions with AND", + condition: `resources.exists(r, r.kind == "ConfigMap") && resources.exists(r, r.kind == "Deployment")`, + resources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: config`, + `apiVersion: apps/v1 +kind: Deployment +metadata: + name: app`, + }, + shouldExecute: true, + }, + { + name: "check nested field", + condition: `resources.exists(r, r.kind == "Deployment" && r.spec.replicas > 2)`, + resources: []string{ + `apiVersion: apps/v1 +kind: Deployment +metadata: + name: app +spec: + replicas: 5`, + }, + shouldExecute: true, + }, + { + name: "check data field in ConfigMap", + condition: `resources.exists(r, r.kind == "ConfigMap" && r.data.environment == "production")`, + resources: []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: env-config +data: + environment: production + region: us-west`, + }, + shouldExecute: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var input []*yaml.RNode + for _, resourceYAML := range tc.resources { + rnode, err := yaml.Parse(resourceYAML) + require.NoError(t, err) + input = append(input, rnode) + } + + evaluator, err := NewCELEvaluator(tc.condition) + require.NoError(t, err) + + result, err := evaluator.EvaluateCondition(ctx, input) + require.NoError(t, err) + assert.Equal(t, tc.shouldExecute, result) + }) + } +} diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go index 8695367ccd..8b01b69cbf 100644 --- a/internal/fnruntime/runner.go +++ b/internal/fnruntime/runner.go @@ -153,7 +153,27 @@ func NewRunner( } } } - return NewFunctionRunner(ctx, fltr, pkgPath, fnResult, fnResults, opts) + + // Initialize CEL evaluator if a condition is specified + var evaluator *CELEvaluator + if f.Condition != "" { + var err error + evaluator, err = NewCELEvaluator(f.Condition) + if err != nil { + return nil, fmt.Errorf("failed to create CEL evaluator: %w", err) + } + } + + fr, err := NewFunctionRunner(ctx, fltr, pkgPath, fnResult, fnResults, opts) + if err != nil { + return nil, err + } + + // Set condition and evaluator + fr.condition = f.Condition + fr.evaluator = evaluator + + return fr, nil } // NewFunctionRunner returns a FunctionRunner given a specification of a function @@ -194,10 +214,32 @@ type FunctionRunner struct { fnResult *fnresult.Result fnResults *fnresult.ResultList opts runneroptions.RunnerOptions + condition string // CEL condition expression + evaluator *CELEvaluator // CEL evaluator for condition checking } func (fr *FunctionRunner) Filter(input []*yaml.RNode) (output []*yaml.RNode, err error) { pr := printer.FromContextOrDie(fr.ctx) + + // Check condition before executing function + if fr.evaluator != nil { + shouldExecute, err := fr.evaluator.EvaluateCondition(fr.ctx, input) + if err != nil { + return nil, fmt.Errorf("failed to evaluate condition for function %q: %w", fr.name, err) + } + + if !shouldExecute { + if !fr.disableCLIOutput { + pr.Printf("[SKIPPED] %q (condition not met)\n", fr.name) + } + // Append a skipped result so consumers get one result per pipeline step + fr.fnResult.ExitCode = 0 + fr.fnResults.Items = append(fr.fnResults.Items, *fr.fnResult) + // Return input unchanged - function is skipped + return input, nil + } + } + if !fr.disableCLIOutput { if fr.opts.AllowWasm { if fr.opts.DisplayResourceCount { diff --git a/pkg/api/kptfile/v1/types.go b/pkg/api/kptfile/v1/types.go index 7d160ed61e..f2559bba74 100644 --- a/pkg/api/kptfile/v1/types.go +++ b/pkg/api/kptfile/v1/types.go @@ -361,6 +361,19 @@ type Function struct { // `Exclude` are used to specify resources on which the function should NOT be executed. // If not specified, all resources selected by `Selectors` are selected. Exclusions []Selector `yaml:"exclude,omitempty" json:"exclude,omitempty"` + + // `Condition` is an optional CEL expression that determines whether this + // 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: + // condition: "resources.filter(r, r.kind == 'Deployment').size() > 0" + Condition string `yaml:"condition,omitempty" json:"condition,omitempty"` } // Selector specifies the selection criteria From 84e1d94d75228282c325da33f1f4ae6b3c26567a Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 11:07:04 +0530 Subject: [PATCH 02/19] fix: remove k8s.io/apiserver dependency from CEL evaluator 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 --- internal/fnruntime/celeval.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/internal/fnruntime/celeval.go b/internal/fnruntime/celeval.go index 71cc9a0f8e..ebc6245b82 100644 --- a/internal/fnruntime/celeval.go +++ b/internal/fnruntime/celeval.go @@ -21,7 +21,6 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" "github.com/google/cel-go/ext" - k8scellib "k8s.io/apiserver/pkg/cel/library" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -41,27 +40,13 @@ type CELEvaluator struct { func NewCELEvaluator(condition string) (*CELEvaluator, error) { env, err := cel.NewEnv( cel.Variable("resources", cel.ListType(cel.DynType)), - // Below is a list of Env settings that is a selection of https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go - // General rules are for maintaining this list. - // 1. utility functions should be available. This allows for more compatibility with k8s's own CEL conditions - // 2. AST validation is not needed as kpt will recompile CEL expressions every time, there is no cost-saving in exiting early - // 3. Compile time optimisations do not make sense, as each CEL expression will be evaluated once before being discarded. - // 3. Things that are helping with authorization in k8s are not needed, as they're returning either ResourceCheck or Decision types, which are not needed for kpt cel.HomogeneousAggregateLiterals(), cel.DefaultUTCTimeZone(true), - k8scellib.URLs(), - k8scellib.Regex(), - k8scellib.Lists(), cel.CrossTypeNumericComparisons(true), cel.OptionalTypes(), - k8scellib.Quantity(), ext.Strings(ext.StringsVersion(2)), ext.Sets(), - k8scellib.IP(), - k8scellib.CIDR(), - k8scellib.Format(), ext.TwoVarComprehensions(), - k8scellib.SemverLib(k8scellib.SemverVersion(1)), ext.Lists(ext.ListsVersion(3)), ) if err != nil { @@ -88,7 +73,6 @@ func NewCELEvaluator(condition string) (*CELEvaluator, error) { prg, err := env.Program(ast, cel.CostLimit(costLimit), cel.InterruptCheckFrequency(checkFrequency), - cel.CostTracking(&k8scellib.CostEstimator{}), ) if err != nil { return nil, fmt.Errorf("failed to create CEL program: %w", err) From 08742aabc89973dbef4b4361b9174eb036830dcc Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 21:42:38 +0530 Subject: [PATCH 03/19] refactor: move CELEnvironment to runneroptions, compile program per EvaluateCondition 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 --- .../condition-met/.expected/config.yaml | 12 ++ .../condition-met/.expected/diff.patch | 33 +++++ .../fn-render/condition/condition-met/Kptfile | 10 ++ .../condition/condition-met/resources.yaml | 13 ++ .../condition-not-met/.expected/config.yaml | 4 + .../condition-not-met/.expected/diff.patch | 13 ++ .../condition/condition-not-met/Kptfile | 10 ++ .../condition-not-met/resources.yaml | 6 + internal/fnruntime/celeval.go | 137 +----------------- internal/fnruntime/conditional_e2e_test.go | 132 ++++------------- internal/fnruntime/runner.go | 32 ++-- pkg/lib/runneroptions/celenv.go | 124 ++++++++++++++++ pkg/lib/runneroptions/runneroptions.go | 11 ++ 13 files changed, 284 insertions(+), 253 deletions(-) create mode 100644 e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml create mode 100644 e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch create mode 100644 e2e/testdata/fn-render/condition/condition-met/Kptfile create mode 100644 e2e/testdata/fn-render/condition/condition-met/resources.yaml create mode 100644 e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml create mode 100644 e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch create mode 100644 e2e/testdata/fn-render/condition/condition-not-met/Kptfile create mode 100644 e2e/testdata/fn-render/condition/condition-not-met/resources.yaml create mode 100644 pkg/lib/runneroptions/celenv.go diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml b/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml new file mode 100644 index 0000000000..e6d8e6ec28 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml @@ -0,0 +1,12 @@ +actualStripLines: + - " stderr: 'WARNING: The requested image''s platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested'" + +stdErrStripLines: + - " Stderr:" + - " \"WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested\"" + +stdErr: | + Package: "condition-met" + [RUNNING] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" + [PASS] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" in 0s + Successfully executed 1 function(s) in 1 package(s). diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch new file mode 100644 index 0000000000..b86067e347 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch @@ -0,0 +1,33 @@ +diff --git a/Kptfile b/Kptfile +index abc1234..def5678 100644 +--- a/Kptfile ++++ b/Kptfile +@@ -7,3 +7,8 @@ pipeline: + configMap: + env: production + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++status: ++ conditions: ++ - type: Rendered ++ status: "True" ++ reason: RenderSuccess +diff --git a/resources.yaml b/resources.yaml +index abc1234..def5678 100644 +--- a/resources.yaml ++++ b/resources.yaml +@@ -3,10 +3,12 @@ kind: ConfigMap + metadata: + name: app-config ++ labels: ++ env: production + data: + key: value + --- + apiVersion: apps/v1 + kind: Deployment + metadata: + name: my-app ++ labels: ++ env: production + spec: + replicas: 1 diff --git a/e2e/testdata/fn-render/condition/condition-met/Kptfile b/e2e/testdata/fn-render/condition/condition-met/Kptfile new file mode 100644 index 0000000000..5053b984ba --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-met/Kptfile @@ -0,0 +1,10 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: app +pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 + configMap: + env: production + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" diff --git a/e2e/testdata/fn-render/condition/condition-met/resources.yaml b/e2e/testdata/fn-render/condition/condition-met/resources.yaml new file mode 100644 index 0000000000..47bec8bc08 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-met/resources.yaml @@ -0,0 +1,13 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config +data: + key: value +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-app +spec: + replicas: 1 diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml b/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml new file mode 100644 index 0000000000..5a060255b5 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml @@ -0,0 +1,4 @@ +stdErr: | + Package: "condition-not-met" + [SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" (condition not met) + Successfully executed 1 function(s) in 1 package(s). diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch new file mode 100644 index 0000000000..42eec149dc --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch @@ -0,0 +1,13 @@ +diff --git a/Kptfile b/Kptfile +index abc1234..def5678 100644 +--- a/Kptfile ++++ b/Kptfile +@@ -7,3 +7,8 @@ pipeline: + configMap: + env: production + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++status: ++ conditions: ++ - type: Rendered ++ status: "True" ++ reason: RenderSuccess diff --git a/e2e/testdata/fn-render/condition/condition-not-met/Kptfile b/e2e/testdata/fn-render/condition/condition-not-met/Kptfile new file mode 100644 index 0000000000..5053b984ba --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-not-met/Kptfile @@ -0,0 +1,10 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: app +pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 + configMap: + env: production + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" diff --git a/e2e/testdata/fn-render/condition/condition-not-met/resources.yaml b/e2e/testdata/fn-render/condition/condition-not-met/resources.yaml new file mode 100644 index 0000000000..c8cdecf359 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-not-met/resources.yaml @@ -0,0 +1,6 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-app +spec: + replicas: 1 diff --git a/internal/fnruntime/celeval.go b/internal/fnruntime/celeval.go index ebc6245b82..4dd19ba761 100644 --- a/internal/fnruntime/celeval.go +++ b/internal/fnruntime/celeval.go @@ -14,137 +14,8 @@ package fnruntime -import ( - "context" - "fmt" +import "github.com/kptdev/kpt/pkg/lib/runneroptions" - "github.com/google/cel-go/cel" - "github.com/google/cel-go/common/types" - "github.com/google/cel-go/ext" - "sigs.k8s.io/kustomize/kyaml/yaml" -) - -const checkFrequency = 100 - -// This gives about .1 seconds of CPU time for the evaluation to run -const costLimit = 1000000 - -// CELEvaluator evaluates CEL expressions against KRM resources -type CELEvaluator struct { - env *cel.Env - prg cel.Program // Pre-compiled program for the condition -} - -// NewCELEvaluator creates a new CEL evaluator with the standard environment -// for the given condition string. -func NewCELEvaluator(condition string) (*CELEvaluator, error) { - env, err := cel.NewEnv( - cel.Variable("resources", cel.ListType(cel.DynType)), - cel.HomogeneousAggregateLiterals(), - cel.DefaultUTCTimeZone(true), - cel.CrossTypeNumericComparisons(true), - cel.OptionalTypes(), - ext.Strings(ext.StringsVersion(2)), - ext.Sets(), - ext.TwoVarComprehensions(), - ext.Lists(ext.ListsVersion(3)), - ) - if err != nil { - return nil, fmt.Errorf("failed to create CEL environment: %w", err) - } - - evaluator := &CELEvaluator{ - env: env, - } - - // Pre-compile the condition if provided - if condition != "" { - ast, issues := env.Compile(condition) - if issues != nil && issues.Err() != nil { - return nil, fmt.Errorf("failed to compile CEL expression: %w", issues.Err()) - } - - // Validate that the expression returns a boolean - if ast.OutputType() != cel.BoolType { - return nil, fmt.Errorf("CEL expression must return a boolean, got %v", ast.OutputType()) - } - - // Create the program with a hard cost limit and cost tracking enabled - prg, err := env.Program(ast, - cel.CostLimit(costLimit), - cel.InterruptCheckFrequency(checkFrequency), - ) - if err != nil { - return nil, fmt.Errorf("failed to create CEL program: %w", err) - } - - evaluator.prg = prg - } - - return evaluator, nil -} - -// EvaluateCondition evaluates a CEL condition expression against a list of resources -// Returns true if the condition is met, false otherwise -// The program is pre-compiled, so this just evaluates it with the given resources -func (e *CELEvaluator) EvaluateCondition(ctx context.Context, resources []*yaml.RNode) (bool, error) { - if e.prg == nil { - return true, nil - } - - // Convert resources to a format suitable for CEL - resourceList, err := e.resourcesToList(resources) - if err != nil { - return false, fmt.Errorf("failed to convert resources: %w", err) - } - - // Evaluate the expression - out, _, err := e.prg.ContextEval(ctx, map[string]interface{}{ - "resources": resourceList, - }) - if err != nil { - return false, fmt.Errorf("failed to evaluate CEL expression: %w", err) - } - - // Extract the boolean result - result, ok := out.(types.Bool) - if !ok { - return false, fmt.Errorf("CEL expression must return a boolean, got %T", out) - } - - return bool(result), nil -} - -// resourcesToList converts RNodes to a list of maps for CEL evaluation -func (e *CELEvaluator) resourcesToList(resources []*yaml.RNode) ([]interface{}, error) { - result := make([]interface{}, 0, len(resources)) - - for _, resource := range resources { - // Convert each resource to a map - resourceMap, err := e.resourceToMap(resource) - if err != nil { - return nil, err - } - result = append(result, resourceMap) - } - - return result, nil -} - -// resourceToMap converts a single RNode to a map for CEL evaluation -// Converts yaml.Node directly to avoid serialization overhead -func (e *CELEvaluator) resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { - // Get the underlying yaml.Node - node := resource.YNode() - if node == nil { - return nil, fmt.Errorf("resource has nil yaml.Node") - } - - // Convert yaml.Node to map[string]interface{} directly - var result map[string]interface{} - if err := node.Decode(&result); err != nil { - return nil, fmt.Errorf("failed to decode resource: %w", err) - } - - return result, nil -} +// CELEvaluator is an alias for runneroptions.CELEnvironment so that runner.go +// can reference it within the fnruntime package without an import cycle. +type CELEvaluator = runneroptions.CELEnvironment diff --git a/internal/fnruntime/conditional_e2e_test.go b/internal/fnruntime/conditional_e2e_test.go index f3c47d2539..122697c40d 100644 --- a/internal/fnruntime/conditional_e2e_test.go +++ b/internal/fnruntime/conditional_e2e_test.go @@ -25,17 +25,18 @@ import ( "github.com/kptdev/kpt/pkg/printer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/yaml" ) // TestFunctionRunner_ConditionalExecution_E2E tests the complete flow -// of conditional function execution +// of conditional function execution using the shared CEL environment. func TestFunctionRunner_ConditionalExecution_E2E(t *testing.T) { ctx := printer.WithContext(context.Background(), printer.New(nil, nil)) - _ = filesys.MakeFsInMemory() // Reserved for future use + + celEnv, err := runneroptions.NewCELEnvironment() + require.NoError(t, err) testCases := []struct { name string @@ -48,12 +49,7 @@ func TestFunctionRunner_ConditionalExecution_E2E(t *testing.T) { name: "condition met - ConfigMap exists", condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, inputResources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: app-config -data: - env: production`, + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: app-config\ndata:\n env: production", }, shouldExecute: true, description: "Function should execute when ConfigMap with specific name exists", @@ -62,12 +58,7 @@ data: name: "condition not met - ConfigMap missing", condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, inputResources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: other-config -data: - env: staging`, + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: other-config\ndata:\n env: staging", }, shouldExecute: false, description: "Function should skip when specified ConfigMap doesn't exist", @@ -76,12 +67,7 @@ data: name: "condition met - Deployment count check", condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, inputResources: []string{ - `apiVersion: apps/v1 -kind: Deployment -metadata: - name: web-app -spec: - replicas: 3`, + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web-app\nspec:\n replicas: 3", }, shouldExecute: true, description: "Function should execute when Deployments exist", @@ -90,35 +76,22 @@ spec: name: "condition not met - no Deployments", condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, inputResources: []string{ - `apiVersion: v1 -kind: Service -metadata: - name: web-service`, + "apiVersion: v1\nkind: Service\nmetadata:\n name: web-service", }, shouldExecute: false, description: "Function should skip when no Deployments exist", }, { - name: "always true condition", - condition: `true`, - inputResources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: test`, - }, + name: "always true condition", + condition: `true`, + inputResources: []string{"apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"}, shouldExecute: true, description: "Function should always execute with true condition", }, { - name: "always false condition", - condition: `false`, - inputResources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: test`, - }, + name: "always false condition", + condition: `false`, + inputResources: []string{"apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"}, shouldExecute: false, description: "Function should never execute with false condition", }, @@ -126,7 +99,6 @@ metadata: for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - // Parse input resources var input []*yaml.RNode for _, resourceYAML := range tc.inputResources { rnode, err := yaml.Parse(resourceYAML) @@ -134,78 +106,54 @@ metadata: input = append(input, rnode) } - // Create a mock function that adds an annotation functionExecuted := false mockFilter := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { functionExecuted = true - // Add an annotation to mark execution for _, node := range nodes { - err := node.PipeE( - yaml.SetAnnotation("test-annotation", "executed"), - ) - if err != nil { + if err := node.PipeE(yaml.SetAnnotation("test-annotation", "executed")); err != nil { return nil, err } } return nodes, nil } - // Create adapter function to match FunctionFilter.Run signature adapterFunc := func(reader io.Reader, writer io.Writer) error { - // Parse YAML from reader into RNodes nodes, err := (&kio.ByteReader{Reader: reader}).Read() if err != nil { return err } - - // Call mockFilter resultNodes, err := mockFilter(nodes) if err != nil { return err } - - // Write results back to writer return (&kio.ByteWriter{Writer: writer}).Write(resultNodes) } - // Create function runner with condition fnResult := &fnresultv1.Result{} fnResults := &fnresultv1.ResultList{} - evaluator, err := NewCELEvaluator(tc.condition) - require.NoError(t, err) - runner := &FunctionRunner{ ctx: ctx, name: "test-function", pkgPath: types.UniquePath("test"), disableCLIOutput: true, - filter: &runtimeutil.FunctionFilter{ - Run: adapterFunc, - }, - fnResult: fnResult, - fnResults: fnResults, - opts: runneroptions.RunnerOptions{}, - condition: tc.condition, - evaluator: evaluator, + filter: &runtimeutil.FunctionFilter{Run: adapterFunc}, + fnResult: fnResult, + fnResults: fnResults, + opts: runneroptions.RunnerOptions{}, + condition: tc.condition, + celEnv: celEnv, } - // Execute the filter output, err := runner.Filter(input) require.NoError(t, err) - // Verify function execution based on condition if tc.shouldExecute { assert.True(t, functionExecuted, tc.description) - // Verify annotation was added - annotations := output[0].GetAnnotations() - annotation := annotations["test-annotation"] - assert.Equal(t, "executed", annotation) + assert.Equal(t, "executed", output[0].GetAnnotations()["test-annotation"]) } else { assert.False(t, functionExecuted, tc.description) - // Verify output is unchanged (no annotation) - annotations := output[0].GetAnnotations() - _, exists := annotations["test-annotation"] + _, exists := output[0].GetAnnotations()["test-annotation"] assert.False(t, exists, "annotation should not exist when function is skipped") } }) @@ -213,9 +161,13 @@ metadata: } // TestFunctionRunner_ConditionalExecution_ComplexConditions tests more advanced CEL expressions +// directly against the shared CEL environment. func TestFunctionRunner_ConditionalExecution_ComplexConditions(t *testing.T) { ctx := context.Background() + celEnv, err := runneroptions.NewCELEnvironment() + require.NoError(t, err) + testCases := []struct { name string condition string @@ -226,14 +178,8 @@ func TestFunctionRunner_ConditionalExecution_ComplexConditions(t *testing.T) { name: "multiple conditions with AND", condition: `resources.exists(r, r.kind == "ConfigMap") && resources.exists(r, r.kind == "Deployment")`, resources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: config`, - `apiVersion: apps/v1 -kind: Deployment -metadata: - name: app`, + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: config", + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: app", }, shouldExecute: true, }, @@ -241,12 +187,7 @@ metadata: name: "check nested field", condition: `resources.exists(r, r.kind == "Deployment" && r.spec.replicas > 2)`, resources: []string{ - `apiVersion: apps/v1 -kind: Deployment -metadata: - name: app -spec: - replicas: 5`, + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: app\nspec:\n replicas: 5", }, shouldExecute: true, }, @@ -254,13 +195,7 @@ spec: name: "check data field in ConfigMap", condition: `resources.exists(r, r.kind == "ConfigMap" && r.data.environment == "production")`, resources: []string{ - `apiVersion: v1 -kind: ConfigMap -metadata: - name: env-config -data: - environment: production - region: us-west`, + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: env-config\ndata:\n environment: production\n region: us-west", }, shouldExecute: true, }, @@ -275,10 +210,7 @@ data: input = append(input, rnode) } - evaluator, err := NewCELEvaluator(tc.condition) - require.NoError(t, err) - - result, err := evaluator.EvaluateCondition(ctx, input) + result, err := celEnv.EvaluateCondition(ctx, tc.condition, input) require.NoError(t, err) assert.Equal(t, tc.shouldExecute, result) }) diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go index 8b01b69cbf..8f1bda17aa 100644 --- a/internal/fnruntime/runner.go +++ b/internal/fnruntime/runner.go @@ -154,25 +154,17 @@ func NewRunner( } } - // Initialize CEL evaluator if a condition is specified - var evaluator *CELEvaluator - if f.Condition != "" { - var err error - evaluator, err = NewCELEvaluator(f.Condition) - if err != nil { - return nil, fmt.Errorf("failed to create CEL evaluator: %w", err) - } - } - fr, err := NewFunctionRunner(ctx, fltr, pkgPath, fnResult, fnResults, opts) if err != nil { return nil, err } - - // Set condition and evaluator - fr.condition = f.Condition - fr.evaluator = evaluator - + + // Set condition; the shared CEL environment from opts is used at evaluation time. + if f.Condition != "" { + fr.condition = f.Condition + fr.celEnv = opts.CELEnvironment + } + return fr, nil } @@ -214,20 +206,20 @@ type FunctionRunner struct { fnResult *fnresult.Result fnResults *fnresult.ResultList opts runneroptions.RunnerOptions - condition string // CEL condition expression - evaluator *CELEvaluator // CEL evaluator for condition checking + condition string // CEL condition expression + celEnv *CELEvaluator // shared CEL environment for condition evaluation } func (fr *FunctionRunner) Filter(input []*yaml.RNode) (output []*yaml.RNode, err error) { pr := printer.FromContextOrDie(fr.ctx) // Check condition before executing function - if fr.evaluator != nil { - shouldExecute, err := fr.evaluator.EvaluateCondition(fr.ctx, input) + 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 { if !fr.disableCLIOutput { pr.Printf("[SKIPPED] %q (condition not met)\n", fr.name) diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go new file mode 100644 index 0000000000..2acbb9a1a7 --- /dev/null +++ b/pkg/lib/runneroptions/celenv.go @@ -0,0 +1,124 @@ +// Copyright 2026 The kpt and Nephio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package runneroptions + +import ( + "context" + "fmt" + + "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/ext" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +const celCheckFrequency = 100 + +// celCostLimit gives about .1 seconds of CPU time for the evaluation to run +const celCostLimit = 1000000 + +// CELEnvironment holds a shared CEL environment for evaluating conditions. +// The environment is created once and reused; programs are compiled per condition call. +type CELEnvironment struct { + env *cel.Env +} + +// NewCELEnvironment creates a new CELEnvironment with the standard KRM variable bindings. +func NewCELEnvironment() (*CELEnvironment, error) { + env, err := cel.NewEnv( + cel.Variable("resources", cel.ListType(cel.DynType)), + cel.HomogeneousAggregateLiterals(), + cel.DefaultUTCTimeZone(true), + cel.CrossTypeNumericComparisons(true), + cel.OptionalTypes(), + ext.Strings(ext.StringsVersion(2)), + ext.Sets(), + ext.TwoVarComprehensions(), + ext.Lists(ext.ListsVersion(3)), + ) + if err != nil { + return nil, fmt.Errorf("failed to create CEL environment: %w", err) + } + return &CELEnvironment{env: env}, nil +} + +// EvaluateCondition compiles and evaluates a CEL condition against a list of KRM resources. +// Returns true if the condition is met, false otherwise. +// An empty condition always returns true (function executes unconditionally). +func (e *CELEnvironment) EvaluateCondition(ctx context.Context, condition string, resources []*yaml.RNode) (bool, error) { + if condition == "" { + return true, nil + } + + ast, issues := e.env.Compile(condition) + if issues != nil && issues.Err() != nil { + return false, fmt.Errorf("failed to compile CEL expression: %w", issues.Err()) + } + + if ast.OutputType() != cel.BoolType { + return false, fmt.Errorf("CEL expression must return a boolean, got %v", ast.OutputType()) + } + + prg, err := e.env.Program(ast, + cel.CostLimit(celCostLimit), + cel.InterruptCheckFrequency(celCheckFrequency), + ) + if err != nil { + return false, fmt.Errorf("failed to create CEL program: %w", err) + } + + resourceList, err := resourcesToList(resources) + if err != nil { + return false, fmt.Errorf("failed to convert resources: %w", err) + } + + out, _, err := prg.ContextEval(ctx, map[string]interface{}{ + "resources": resourceList, + }) + if err != nil { + return false, fmt.Errorf("failed to evaluate CEL expression: %w", err) + } + + result, ok := out.(types.Bool) + if !ok { + return false, fmt.Errorf("CEL expression must return a boolean, got %T", out) + } + + return bool(result), nil +} + +func resourcesToList(resources []*yaml.RNode) ([]interface{}, error) { + result := make([]interface{}, 0, len(resources)) + for _, resource := range resources { + m, err := resourceToMap(resource) + if err != nil { + return nil, err + } + result = append(result, m) + } + return result, nil +} + +func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { + node := resource.YNode() + if node == nil { + return nil, fmt.Errorf("resource has nil yaml.Node") + } + var result map[string]interface{} + if err := node.Decode(&result); err != nil { + return nil, fmt.Errorf("failed to decode resource: %w", err) + } + return result, nil +} diff --git a/pkg/lib/runneroptions/runneroptions.go b/pkg/lib/runneroptions/runneroptions.go index 0f6fa19615..86f4f602bf 100644 --- a/pkg/lib/runneroptions/runneroptions.go +++ b/pkg/lib/runneroptions/runneroptions.go @@ -58,11 +58,22 @@ type RunnerOptions struct { // ResolveToImage will resolve a partial image to a fully-qualified one ResolveToImage ImageResolveFunc + + // CELEnvironment is the shared CEL environment used to evaluate function conditions. + // It is initialised by InitDefaults and reused across all function runners. + CELEnvironment *CELEnvironment } 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 } // ResolveToImageForCLIFunc returns a func that converts the KRM function short path to the full image url. From df38833bfe1bd0b3f87c56ab1cba9371e404b9ec Mon Sep 17 00:00:00 2001 From: SurbhiAgarwal Date: Thu, 19 Mar 2026 22:05:28 +0530 Subject: [PATCH 04/19] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Surbhi --- internal/fnruntime/celeval_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/fnruntime/celeval_test.go b/internal/fnruntime/celeval_test.go index e55c2abc1f..dece1dadc0 100644 --- a/internal/fnruntime/celeval_test.go +++ b/internal/fnruntime/celeval_test.go @@ -21,47 +21,47 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/kyaml/yaml" + + "github.com/GoogleContainerTools/kpt/internal/fnruntime/runneroptions" ) func TestNewCELEvaluator(t *testing.T) { - eval, err := NewCELEvaluator("true") + env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - assert.NotNil(t, eval) - assert.NotNil(t, eval.env) - assert.NotNil(t, eval.prg) + assert.NotNil(t, env) + assert.NotNil(t, env.Env) } func TestNewCELEvaluator_EmptyCondition(t *testing.T) { - eval, err := NewCELEvaluator("") + env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - assert.NotNil(t, eval) - assert.NotNil(t, eval.env) - assert.Nil(t, eval.prg) + assert.NotNil(t, env) + assert.NotNil(t, env.Env) } func TestEvaluateCondition_EmptyCondition(t *testing.T) { - eval, err := NewCELEvaluator("") + env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - result, err := eval.EvaluateCondition(context.Background(), nil) + result, err := env.EvaluateCondition(context.Background(), "", nil) require.NoError(t, err) assert.True(t, result, "empty condition should return true") } func TestEvaluateCondition_SimpleTrue(t *testing.T) { - eval, err := NewCELEvaluator("true") + env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - result, err := eval.EvaluateCondition(context.Background(), nil) + result, err := env.EvaluateCondition(context.Background(), "true", nil) require.NoError(t, err) assert.True(t, result) } func TestEvaluateCondition_SimpleFalse(t *testing.T) { - eval, err := NewCELEvaluator("false") + env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - result, err := eval.EvaluateCondition(context.Background(), nil) + result, err := env.EvaluateCondition(context.Background(), "false", nil) require.NoError(t, err) assert.False(t, result) } From 6d13e920033fe97a34f66eae7847f0cc1c60988f Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 22:11:38 +0530 Subject: [PATCH 05/19] refactor: replace Go unit e2e test with proper fn-render testdata 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 --- internal/fnruntime/conditional_e2e_test.go | 218 --------------------- 1 file changed, 218 deletions(-) delete mode 100644 internal/fnruntime/conditional_e2e_test.go diff --git a/internal/fnruntime/conditional_e2e_test.go b/internal/fnruntime/conditional_e2e_test.go deleted file mode 100644 index 122697c40d..0000000000 --- a/internal/fnruntime/conditional_e2e_test.go +++ /dev/null @@ -1,218 +0,0 @@ -// Copyright 2026 The kpt and Nephio Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package fnruntime - -import ( - "context" - "io" - "testing" - - "github.com/kptdev/kpt/internal/types" - fnresultv1 "github.com/kptdev/kpt/pkg/api/fnresult/v1" - "github.com/kptdev/kpt/pkg/lib/runneroptions" - "github.com/kptdev/kpt/pkg/printer" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil" - "sigs.k8s.io/kustomize/kyaml/kio" - "sigs.k8s.io/kustomize/kyaml/yaml" -) - -// TestFunctionRunner_ConditionalExecution_E2E tests the complete flow -// of conditional function execution using the shared CEL environment. -func TestFunctionRunner_ConditionalExecution_E2E(t *testing.T) { - ctx := printer.WithContext(context.Background(), printer.New(nil, nil)) - - celEnv, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) - - testCases := []struct { - name string - condition string - inputResources []string - shouldExecute bool - description string - }{ - { - name: "condition met - ConfigMap exists", - condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, - inputResources: []string{ - "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: app-config\ndata:\n env: production", - }, - shouldExecute: true, - description: "Function should execute when ConfigMap with specific name exists", - }, - { - name: "condition not met - ConfigMap missing", - condition: `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "app-config")`, - inputResources: []string{ - "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: other-config\ndata:\n env: staging", - }, - shouldExecute: false, - description: "Function should skip when specified ConfigMap doesn't exist", - }, - { - name: "condition met - Deployment count check", - condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, - inputResources: []string{ - "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web-app\nspec:\n replicas: 3", - }, - shouldExecute: true, - description: "Function should execute when Deployments exist", - }, - { - name: "condition not met - no Deployments", - condition: `resources.filter(r, r.kind == "Deployment").size() > 0`, - inputResources: []string{ - "apiVersion: v1\nkind: Service\nmetadata:\n name: web-service", - }, - shouldExecute: false, - description: "Function should skip when no Deployments exist", - }, - { - name: "always true condition", - condition: `true`, - inputResources: []string{"apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"}, - shouldExecute: true, - description: "Function should always execute with true condition", - }, - { - name: "always false condition", - condition: `false`, - inputResources: []string{"apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test"}, - shouldExecute: false, - description: "Function should never execute with false condition", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var input []*yaml.RNode - for _, resourceYAML := range tc.inputResources { - rnode, err := yaml.Parse(resourceYAML) - require.NoError(t, err) - input = append(input, rnode) - } - - functionExecuted := false - mockFilter := func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { - functionExecuted = true - for _, node := range nodes { - if err := node.PipeE(yaml.SetAnnotation("test-annotation", "executed")); err != nil { - return nil, err - } - } - return nodes, nil - } - - adapterFunc := func(reader io.Reader, writer io.Writer) error { - nodes, err := (&kio.ByteReader{Reader: reader}).Read() - if err != nil { - return err - } - resultNodes, err := mockFilter(nodes) - if err != nil { - return err - } - return (&kio.ByteWriter{Writer: writer}).Write(resultNodes) - } - - fnResult := &fnresultv1.Result{} - fnResults := &fnresultv1.ResultList{} - - runner := &FunctionRunner{ - ctx: ctx, - name: "test-function", - pkgPath: types.UniquePath("test"), - disableCLIOutput: true, - filter: &runtimeutil.FunctionFilter{Run: adapterFunc}, - fnResult: fnResult, - fnResults: fnResults, - opts: runneroptions.RunnerOptions{}, - condition: tc.condition, - celEnv: celEnv, - } - - output, err := runner.Filter(input) - require.NoError(t, err) - - if tc.shouldExecute { - assert.True(t, functionExecuted, tc.description) - assert.Equal(t, "executed", output[0].GetAnnotations()["test-annotation"]) - } else { - assert.False(t, functionExecuted, tc.description) - _, exists := output[0].GetAnnotations()["test-annotation"] - assert.False(t, exists, "annotation should not exist when function is skipped") - } - }) - } -} - -// TestFunctionRunner_ConditionalExecution_ComplexConditions tests more advanced CEL expressions -// directly against the shared CEL environment. -func TestFunctionRunner_ConditionalExecution_ComplexConditions(t *testing.T) { - ctx := context.Background() - - celEnv, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) - - testCases := []struct { - name string - condition string - resources []string - shouldExecute bool - }{ - { - name: "multiple conditions with AND", - condition: `resources.exists(r, r.kind == "ConfigMap") && resources.exists(r, r.kind == "Deployment")`, - resources: []string{ - "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: config", - "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: app", - }, - shouldExecute: true, - }, - { - name: "check nested field", - condition: `resources.exists(r, r.kind == "Deployment" && r.spec.replicas > 2)`, - resources: []string{ - "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: app\nspec:\n replicas: 5", - }, - shouldExecute: true, - }, - { - name: "check data field in ConfigMap", - condition: `resources.exists(r, r.kind == "ConfigMap" && r.data.environment == "production")`, - resources: []string{ - "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: env-config\ndata:\n environment: production\n region: us-west", - }, - shouldExecute: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var input []*yaml.RNode - for _, resourceYAML := range tc.resources { - rnode, err := yaml.Parse(resourceYAML) - require.NoError(t, err) - input = append(input, rnode) - } - - result, err := celEnv.EvaluateCondition(ctx, tc.condition, input) - require.NoError(t, err) - assert.Equal(t, tc.shouldExecute, result) - }) - } -} From 5573f7a861ae86e887690400727ffcc03d1c9018 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 22:25:29 +0530 Subject: [PATCH 06/19] fix: remove k8s.io/apiserver from go.mod, fix celeval_test.go imports - 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 --- go.mod | 1 - go.sum | 2 - internal/fnruntime/celeval_test.go | 144 ++++++++--------------------- 3 files changed, 39 insertions(+), 108 deletions(-) diff --git a/go.mod b/go.mod index 0b467aaaf1..f931aea95d 100644 --- a/go.mod +++ b/go.mod @@ -29,7 +29,6 @@ require ( k8s.io/api v0.34.1 k8s.io/apiextensions-apiserver v0.34.1 k8s.io/apimachinery v0.34.1 - k8s.io/apiserver v0.34.1 k8s.io/cli-runtime v0.34.1 k8s.io/client-go v0.34.1 k8s.io/component-base v0.34.1 diff --git a/go.sum b/go.sum index fe538b32d5..768509ea1a 100644 --- a/go.sum +++ b/go.sum @@ -326,8 +326,6 @@ k8s.io/apiextensions-apiserver v0.34.1 h1:NNPBva8FNAPt1iSVwIE0FsdrVriRXMsaWFMqJb k8s.io/apiextensions-apiserver v0.34.1/go.mod h1:hP9Rld3zF5Ay2Of3BeEpLAToP+l4s5UlxiHfqRaRcMc= k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= -k8s.io/apiserver v0.34.1 h1:U3JBGdgANK3dfFcyknWde1G6X1F4bg7PXuvlqt8lITA= -k8s.io/apiserver v0.34.1/go.mod h1:eOOc9nrVqlBI1AFCvVzsob0OxtPZUCPiUJL45JOTBG0= k8s.io/cli-runtime v0.34.1 h1:btlgAgTrYd4sk8vJTRG6zVtqBKt9ZMDeQZo2PIzbL7M= k8s.io/cli-runtime v0.34.1/go.mod h1:aVA65c+f0MZiMUPbseU/M9l1Wo2byeaGwUuQEQVVveE= k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY= diff --git a/internal/fnruntime/celeval_test.go b/internal/fnruntime/celeval_test.go index dece1dadc0..12340c7c0b 100644 --- a/internal/fnruntime/celeval_test.go +++ b/internal/fnruntime/celeval_test.go @@ -18,182 +18,116 @@ import ( "context" "testing" + "github.com/kptdev/kpt/pkg/lib/runneroptions" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/kyaml/yaml" - - "github.com/GoogleContainerTools/kpt/internal/fnruntime/runneroptions" ) -func TestNewCELEvaluator(t *testing.T) { +func newTestEnv(t *testing.T) *runneroptions.CELEnvironment { + t.Helper() env, err := runneroptions.NewCELEnvironment() require.NoError(t, err) - assert.NotNil(t, env) - assert.NotNil(t, env.Env) + return env } -func TestNewCELEvaluator_EmptyCondition(t *testing.T) { - env, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) +func TestNewCELEnvironment(t *testing.T) { + env := newTestEnv(t) assert.NotNil(t, env) - assert.NotNil(t, env.Env) } func TestEvaluateCondition_EmptyCondition(t *testing.T) { - env, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) - + env := newTestEnv(t) result, err := env.EvaluateCondition(context.Background(), "", nil) require.NoError(t, err) assert.True(t, result, "empty condition should return true") } func TestEvaluateCondition_SimpleTrue(t *testing.T) { - env, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) - + env := newTestEnv(t) result, err := env.EvaluateCondition(context.Background(), "true", nil) require.NoError(t, err) assert.True(t, result) } func TestEvaluateCondition_SimpleFalse(t *testing.T) { - env, err := runneroptions.NewCELEnvironment() - require.NoError(t, err) - + env := newTestEnv(t) result, err := env.EvaluateCondition(context.Background(), "false", nil) require.NoError(t, err) assert.False(t, result) } 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) + env := newTestEnv(t) + + configMap, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config\ndata:\n key: value") + require.NoError(t, err) + deployment, err := yaml.Parse("apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: test-deployment\nspec:\n replicas: 3") 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) + result, err := env.EvaluateCondition(context.Background(), + `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "test-config")`, resources) require.NoError(t, err) - assert.True(t, result, "should find the ConfigMap") + assert.True(t, result) - // 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) + result, err = env.EvaluateCondition(context.Background(), + `resources.exists(r, r.kind == "ConfigMap" && r.metadata.name == "wrong-name")`, resources) require.NoError(t, err) - assert.False(t, result, "should not find ConfigMap with wrong name") + assert.False(t, result) - // 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) + result, err = env.EvaluateCondition(context.Background(), + `resources.exists(r, r.kind == "Deployment")`, resources) require.NoError(t, err) - assert.True(t, result, "should find the Deployment") + assert.True(t, result) } func TestEvaluateCondition_ResourceCount(t *testing.T) { - // Create test resources - deploymentYAML := ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: test-deployment -spec: - replicas: 3 -` + env := newTestEnv(t) - deployment, err := yaml.Parse(deploymentYAML) + deployment, err := yaml.Parse("apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: test-deployment\nspec:\n replicas: 3") 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) + result, err := env.EvaluateCondition(context.Background(), + `resources.filter(r, r.kind == "Deployment").size() > 0`, resources) require.NoError(t, err) - assert.True(t, result, "should find deployments") + assert.True(t, result) - // 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) + result, err = env.EvaluateCondition(context.Background(), + `resources.filter(r, r.kind == "ConfigMap").size() == 0`, resources) require.NoError(t, err) - assert.True(t, result, "should not find ConfigMaps") + assert.True(t, result) } func TestEvaluateCondition_InvalidExpression(t *testing.T) { - // Test invalid syntax - _, err := NewCELEvaluator("this is not valid CEL") + env := newTestEnv(t) + _, err := env.EvaluateCondition(context.Background(), "this is not valid CEL", nil) assert.Error(t, err) 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") + env := newTestEnv(t) + _, err := env.EvaluateCondition(context.Background(), "1 + 1", nil) assert.Error(t, err) assert.Contains(t, err.Error(), "must return a boolean") } -// TestEvaluateCondition_Immutability ensures CEL evaluation cannot mutate the input resources func TestEvaluateCondition_Immutability(t *testing.T) { - configMapYAML := ` -apiVersion: v1 -kind: ConfigMap -metadata: - name: test-config - namespace: default -data: - key: original-value -` + env := newTestEnv(t) - configMap, err := yaml.Parse(configMapYAML) + configMap, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test-config\n namespace: default\ndata:\n key: original-value") 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) + _, err = env.EvaluateCondition(context.Background(), + `resources.exists(r, r.kind == "ConfigMap")`, []*yaml.RNode{configMap}) 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") From 2f777f8cc3c020b4b537f75807e4ff12c9e3e10b Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 23:14:39 +0530 Subject: [PATCH 07/19] fix: nil CELEnvironment in cmdeval test struct comparison RunnerOptions.CELEnvironment is now populated by InitDefaults, so nil it out before struct comparison just like ResolveToImage. Signed-off-by: Surbhi --- thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go b/thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go index a7e862dfc8..78ad390ba1 100644 --- a/thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go +++ b/thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go @@ -432,6 +432,7 @@ apiVersion: v1 r.runFns.Function = nil r.runFns.FnConfig = nil r.runFns.RunnerOptions.ResolveToImage = nil + r.runFns.RunnerOptions.CELEnvironment = nil tt.expectedStruct.FnConfigPath = tt.fnConfigPath if !assert.Equal(t, *tt.expectedStruct, r.runFns) { t.FailNow() From 259e803b38e94ffc546741e365dd62ed531d6129 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 19 Mar 2026 23:36:12 +0530 Subject: [PATCH 08/19] fix: two e2e test failures in condition testdata 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 --- .../fn-render/condition/condition-met/.krmignore | 1 + .../fn-render/condition/condition-not-met/.krmignore | 1 + pkg/lib/runneroptions/celenv.go | 11 +++++++++++ 3 files changed, 13 insertions(+) create mode 100644 e2e/testdata/fn-render/condition/condition-met/.krmignore create mode 100644 e2e/testdata/fn-render/condition/condition-not-met/.krmignore diff --git a/e2e/testdata/fn-render/condition/condition-met/.krmignore b/e2e/testdata/fn-render/condition/condition-met/.krmignore new file mode 100644 index 0000000000..9d7a4007d6 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-met/.krmignore @@ -0,0 +1 @@ +.expected diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.krmignore b/e2e/testdata/fn-render/condition/condition-not-met/.krmignore new file mode 100644 index 0000000000..9d7a4007d6 --- /dev/null +++ b/e2e/testdata/fn-render/condition/condition-not-met/.krmignore @@ -0,0 +1 @@ +.expected diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index 2acbb9a1a7..2a9d849081 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -120,5 +120,16 @@ func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { if err := node.Decode(&result); err != nil { return nil, fmt.Errorf("failed to decode resource: %w", err) } + // Ensure standard KRM fields are always present so CEL expressions like + // r.kind == "Deployment" never error with "no such key". + if _, ok := result["apiVersion"]; !ok { + result["apiVersion"] = "" + } + if _, ok := result["kind"]; !ok { + result["kind"] = "" + } + if _, ok := result["metadata"]; !ok { + result["metadata"] = map[string]interface{}{} + } return result, nil } From 728c384d42883059e5d13eb836b1effe1b443f62 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Sat, 21 Mar 2026 22:30:36 +0530 Subject: [PATCH 09/19] fix: update condition testdata diff.patch with correct expected output Signed-off-by: Surbhi --- .../condition-met/.expected/diff.patch | 26 +++++++++++++++---- .../condition-not-met/.expected/diff.patch | 10 ++++--- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch index b86067e347..991a8ca157 100644 --- a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch @@ -1,21 +1,30 @@ diff --git a/Kptfile b/Kptfile -index abc1234..def5678 100644 +index 5053b98..8d8d336 100644 --- a/Kptfile +++ b/Kptfile -@@ -7,3 +7,8 @@ pipeline: +@@ -3,7 +3,9 @@ kind: Kptfile + metadata: + name: app + pipeline: ++ labels: ++ env: production ++pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 configMap: env: production - condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') +status: + conditions: + - type: Rendered + status: "True" + reason: RenderSuccess diff --git a/resources.yaml b/resources.yaml -index abc1234..def5678 100644 +index 47bec8b..5f18ce1 100644 --- a/resources.yaml +++ b/resources.yaml -@@ -3,10 +3,12 @@ kind: ConfigMap +@@ -3,10 +3,22 @@ kind: ConfigMap metadata: name: app-config + labels: @@ -31,3 +40,10 @@ index abc1234..def5678 100644 + env: production spec: replicas: 1 ++ selector: ++ matchLabels: ++ env: production ++ template: ++ metadata: ++ labels: ++ env: production diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch index 42eec149dc..933d5a4816 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch @@ -1,11 +1,15 @@ diff --git a/Kptfile b/Kptfile -index abc1234..def5678 100644 +index 5053b98..b29e7dd 100644 --- a/Kptfile +++ b/Kptfile -@@ -7,3 +7,8 @@ pipeline: +@@ -5,5 +5,10 @@ metadata: + pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 configMap: env: production - condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') +status: + conditions: + - type: Rendered From caebfa46be80b297a1f0d1dde5386527c1daa54d Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 26 Mar 2026 11:08:44 +0530 Subject: [PATCH 10/19] fix: correct e2e testdata and clarify k8s CEL library decision - 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 --- .../condition-met/.expected/config.yaml | 4 +- .../condition-met/.expected/diff.patch | 44 ++----------------- .../fn-render/condition/condition-met/Kptfile | 4 +- .../condition-not-met/.expected/config.yaml | 9 +++- .../condition-not-met/.expected/diff.patch | 12 ++--- .../condition/condition-not-met/Kptfile | 4 +- pkg/lib/runneroptions/celenv.go | 4 ++ 7 files changed, 24 insertions(+), 57 deletions(-) diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml b/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml index e6d8e6ec28..318c6710c8 100644 --- a/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/config.yaml @@ -7,6 +7,6 @@ stdErrStripLines: stdErr: | Package: "condition-met" - [RUNNING] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" - [PASS] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" in 0s + [RUNNING] "ghcr.io/kptdev/krm-functions-catalog/no-op" + [PASS] "ghcr.io/kptdev/krm-functions-catalog/no-op" in 0s Successfully executed 1 function(s) in 1 package(s). diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch index 991a8ca157..abd148a4a4 100644 --- a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch @@ -1,49 +1,13 @@ diff --git a/Kptfile b/Kptfile -index 5053b98..8d8d336 100644 +index eb90ac3..7e77aa5 100644 --- a/Kptfile +++ b/Kptfile -@@ -3,7 +3,9 @@ kind: Kptfile - metadata: - name: app - pipeline: -+ labels: -+ env: production -+pipeline: +@@ -6,3 +6,8 @@ pipeline: mutators: - - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 - configMap: - env: production -- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" -+ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') + - image: ghcr.io/kptdev/krm-functions-catalog/no-op + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +status: + conditions: + - type: Rendered + status: "True" + reason: RenderSuccess -diff --git a/resources.yaml b/resources.yaml -index 47bec8b..5f18ce1 100644 ---- a/resources.yaml -+++ b/resources.yaml -@@ -3,10 +3,22 @@ kind: ConfigMap - metadata: - name: app-config -+ labels: -+ env: production - data: - key: value - --- - apiVersion: apps/v1 - kind: Deployment - metadata: - name: my-app -+ labels: -+ env: production - spec: - replicas: 1 -+ selector: -+ matchLabels: -+ env: production -+ template: -+ metadata: -+ labels: -+ env: production diff --git a/e2e/testdata/fn-render/condition/condition-met/Kptfile b/e2e/testdata/fn-render/condition/condition-met/Kptfile index 5053b984ba..eb90ac3a41 100644 --- a/e2e/testdata/fn-render/condition/condition-met/Kptfile +++ b/e2e/testdata/fn-render/condition/condition-met/Kptfile @@ -4,7 +4,5 @@ metadata: name: app pipeline: mutators: - - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 - configMap: - env: production + - image: ghcr.io/kptdev/krm-functions-catalog/no-op condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml b/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml index 5a060255b5..cfd2519544 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/config.yaml @@ -1,4 +1,11 @@ +actualStripLines: + - " stderr: 'WARNING: The requested image''s platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested'" + +stdErrStripLines: + - " Stderr:" + - " \"WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested\"" + stdErr: | Package: "condition-not-met" - [SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5" (condition not met) + [SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/no-op" (condition not met) Successfully executed 1 function(s) in 1 package(s). diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch index 933d5a4816..abd148a4a4 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch @@ -1,15 +1,11 @@ diff --git a/Kptfile b/Kptfile -index 5053b98..b29e7dd 100644 +index eb90ac3..7e77aa5 100644 --- a/Kptfile +++ b/Kptfile -@@ -5,5 +5,10 @@ metadata: - pipeline: +@@ -6,3 +6,8 @@ pipeline: mutators: - - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 - configMap: - env: production -- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" -+ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') + - image: ghcr.io/kptdev/krm-functions-catalog/no-op + condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +status: + conditions: + - type: Rendered diff --git a/e2e/testdata/fn-render/condition/condition-not-met/Kptfile b/e2e/testdata/fn-render/condition/condition-not-met/Kptfile index 5053b984ba..eb90ac3a41 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/Kptfile +++ b/e2e/testdata/fn-render/condition/condition-not-met/Kptfile @@ -4,7 +4,5 @@ metadata: name: app pipeline: mutators: - - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:v0.1.5 - configMap: - env: production + - image: ghcr.io/kptdev/krm-functions-catalog/no-op condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index 2a9d849081..79ec09e94e 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -36,6 +36,10 @@ type CELEnvironment struct { } // NewCELEnvironment creates a new CELEnvironment with the standard KRM variable bindings. +// It includes cel-go built-in extensions for strings, sets, lists and comprehensions. +// Note: k8s.io/apiserver CEL library extensions (IP, CIDR, Quantity, SemVer) are intentionally +// excluded because that dependency causes significant binary size increases and CI build failures. +// The cel-go built-in extensions are sufficient for KRM resource filtering use cases. func NewCELEnvironment() (*CELEnvironment, error) { env, err := cel.NewEnv( cel.Variable("resources", cel.ListType(cel.DynType)), From 6d1fb6bec75c8eaf410b22b05e497475a4c2c7dc Mon Sep 17 00:00:00 2001 From: Surbhi Date: Thu, 26 Mar 2026 11:27:51 +0530 Subject: [PATCH 11/19] fix: update diff.patch with exact hashes from CI output Signed-off-by: Surbhi --- .../condition/condition-met/.expected/diff.patch | 8 +++++--- .../condition/condition-not-met/.expected/diff.patch | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch index abd148a4a4..f24df5b132 100644 --- a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch @@ -1,11 +1,13 @@ diff --git a/Kptfile b/Kptfile -index eb90ac3..7e77aa5 100644 +index eb90ac3..ace574a 100644 --- a/Kptfile +++ b/Kptfile -@@ -6,3 +6,8 @@ pipeline: +@@ -5,4 +5,9 @@ metadata: + pipeline: mutators: - image: ghcr.io/kptdev/krm-functions-catalog/no-op - condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') +status: + conditions: + - type: Rendered diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch index abd148a4a4..f24df5b132 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch @@ -1,11 +1,13 @@ diff --git a/Kptfile b/Kptfile -index eb90ac3..7e77aa5 100644 +index eb90ac3..ace574a 100644 --- a/Kptfile +++ b/Kptfile -@@ -6,3 +6,8 @@ pipeline: +@@ -5,4 +5,9 @@ metadata: + pipeline: mutators: - image: ghcr.io/kptdev/krm-functions-catalog/no-op - condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" +- condition: "resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config')" ++ condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') +status: + conditions: + - type: Rendered From 17828499ed85f3613fd72eeb9458da6971f0db51 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Sun, 29 Mar 2026 02:05:07 +0530 Subject: [PATCH 12/19] feat: add k8s.io/apiserver CEL library extensions (IP, CIDR, Quantity, 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 --- go.mod | 1 + go.sum | 2 ++ pkg/lib/runneroptions/celenv.go | 11 +++++++---- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index f931aea95d..7972891169 100644 --- a/go.mod +++ b/go.mod @@ -133,6 +133,7 @@ require ( 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 k8s.io/component-helpers v0.34.1 // indirect k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect diff --git a/go.sum b/go.sum index 768509ea1a..fe538b32d5 100644 --- a/go.sum +++ b/go.sum @@ -326,6 +326,8 @@ k8s.io/apiextensions-apiserver v0.34.1 h1:NNPBva8FNAPt1iSVwIE0FsdrVriRXMsaWFMqJb k8s.io/apiextensions-apiserver v0.34.1/go.mod h1:hP9Rld3zF5Ay2Of3BeEpLAToP+l4s5UlxiHfqRaRcMc= k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= +k8s.io/apiserver v0.34.1 h1:U3JBGdgANK3dfFcyknWde1G6X1F4bg7PXuvlqt8lITA= +k8s.io/apiserver v0.34.1/go.mod h1:eOOc9nrVqlBI1AFCvVzsob0OxtPZUCPiUJL45JOTBG0= k8s.io/cli-runtime v0.34.1 h1:btlgAgTrYd4sk8vJTRG6zVtqBKt9ZMDeQZo2PIzbL7M= k8s.io/cli-runtime v0.34.1/go.mod h1:aVA65c+f0MZiMUPbseU/M9l1Wo2byeaGwUuQEQVVveE= k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY= diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index 79ec09e94e..5b27efdc60 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -21,6 +21,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" "github.com/google/cel-go/ext" + k8scellib "k8s.io/apiserver/pkg/cel/library" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -36,10 +37,8 @@ type CELEnvironment struct { } // NewCELEnvironment creates a new CELEnvironment with the standard KRM variable bindings. -// It includes cel-go built-in extensions for strings, sets, lists and comprehensions. -// Note: k8s.io/apiserver CEL library extensions (IP, CIDR, Quantity, SemVer) are intentionally -// excluded because that dependency causes significant binary size increases and CI build failures. -// The cel-go built-in extensions are sufficient for KRM resource filtering use cases. +// Includes cel-go built-in extensions and k8s-specific validators (IP, CIDR, Quantity, SemVer) +// from k8s.io/apiserver/pkg/cel/library for full Kubernetes CEL compatibility. func NewCELEnvironment() (*CELEnvironment, error) { env, err := cel.NewEnv( cel.Variable("resources", cel.ListType(cel.DynType)), @@ -51,6 +50,10 @@ func NewCELEnvironment() (*CELEnvironment, error) { ext.Sets(), ext.TwoVarComprehensions(), ext.Lists(ext.ListsVersion(3)), + k8scellib.IP(), + k8scellib.CIDR(), + k8scellib.Quantity(), + k8scellib.SemVer(), ) if err != nil { return nil, fmt.Errorf("failed to create CEL environment: %w", err) From f08fcb86c11d0eacf4c2161107901079d15ead09 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Sun, 29 Mar 2026 02:10:19 +0530 Subject: [PATCH 13/19] fix: correct SemVer function name to SemverLib in k8s CEL library Signed-off-by: Surbhi --- pkg/lib/runneroptions/celenv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index 5b27efdc60..5c7f5fce50 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -53,7 +53,7 @@ func NewCELEnvironment() (*CELEnvironment, error) { k8scellib.IP(), k8scellib.CIDR(), k8scellib.Quantity(), - k8scellib.SemVer(), + k8scellib.SemverLib(), ) if err != nil { return nil, fmt.Errorf("failed to create CEL environment: %w", err) From 2f3382c713f9727978823391e71c1c5fed1ceb3d Mon Sep 17 00:00:00 2001 From: Surbhi Date: Sun, 29 Mar 2026 02:43:47 +0530 Subject: [PATCH 14/19] fix: increase reconcile-timeout from 2m to 5m in live-apply e2e tests 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 --- e2e/testdata/live-apply/apply-depends-on/config.yaml | 2 +- e2e/testdata/live-apply/json-output/config.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/testdata/live-apply/apply-depends-on/config.yaml b/e2e/testdata/live-apply/apply-depends-on/config.yaml index c6a5e5418b..f9167da311 100644 --- a/e2e/testdata/live-apply/apply-depends-on/config.yaml +++ b/e2e/testdata/live-apply/apply-depends-on/config.yaml @@ -17,7 +17,7 @@ parallel: true kptArgs: - "live" - "apply" - - "--reconcile-timeout=2m" + - "--reconcile-timeout=5m" stdOut: | inventory update started diff --git a/e2e/testdata/live-apply/json-output/config.yaml b/e2e/testdata/live-apply/json-output/config.yaml index 6ea446894b..c2d1e3d822 100644 --- a/e2e/testdata/live-apply/json-output/config.yaml +++ b/e2e/testdata/live-apply/json-output/config.yaml @@ -17,7 +17,7 @@ kptArgs: - "live" - "apply" - "--output=json" - - "--reconcile-timeout=2m" + - "--reconcile-timeout=5m" stdOut: | {"action":"Inventory","status":"Started","timestamp":"","type":"group"} {"action":"Inventory","status":"Finished","timestamp":"","type":"group"} From bbf9321c6bbff53c0832a61cb838bae0e15592f6 Mon Sep 17 00:00:00 2001 From: Surbhi Date: Sun, 29 Mar 2026 15:42:10 +0530 Subject: [PATCH 15/19] fix: address Copilot review comments - 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 --- internal/fnruntime/celeval.go | 4 ++-- internal/fnruntime/celeval_test.go | 26 ++++++++++++++++++++++++++ internal/fnruntime/runner.go | 3 +++ pkg/api/kptfile/v1/types.go | 11 ++++++----- pkg/lib/runneroptions/celenv.go | 18 ++++++++++++++++-- 5 files changed, 53 insertions(+), 9 deletions(-) diff --git a/internal/fnruntime/celeval.go b/internal/fnruntime/celeval.go index 4dd19ba761..e578bea5d1 100644 --- a/internal/fnruntime/celeval.go +++ b/internal/fnruntime/celeval.go @@ -16,6 +16,6 @@ package fnruntime import "github.com/kptdev/kpt/pkg/lib/runneroptions" -// 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. type CELEvaluator = runneroptions.CELEnvironment diff --git a/internal/fnruntime/celeval_test.go b/internal/fnruntime/celeval_test.go index 12340c7c0b..e25d444421 100644 --- a/internal/fnruntime/celeval_test.go +++ b/internal/fnruntime/celeval_test.go @@ -132,3 +132,29 @@ func TestEvaluateCondition_Immutability(t *testing.T) { require.NoError(t, err) assert.Equal(t, originalYAML, afterYAML, "CEL evaluation should not mutate input resources") } + +func TestEvaluateCondition_MissingMetadata(t *testing.T) { + env := newTestEnv(t) + + // Resource with no metadata at all + noMetadata, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\ndata:\n key: value") + require.NoError(t, err) + + // Resource with metadata but no name + noName, err := yaml.Parse("apiVersion: v1\nkind: ConfigMap\nmetadata: {}\ndata:\n key: other") + require.NoError(t, err) + + resources := []*yaml.RNode{noMetadata, noName} + + // Should not error — missing metadata.name defaults to "" + 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") + + // kind check should still work + result, err = env.EvaluateCondition(context.Background(), + `resources.exists(r, r.kind == "ConfigMap")`, resources) + require.NoError(t, err) + assert.True(t, result) +} diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go index 8f1bda17aa..f84c87121f 100644 --- a/internal/fnruntime/runner.go +++ b/internal/fnruntime/runner.go @@ -161,6 +161,9 @@ func NewRunner( // 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) + } fr.condition = f.Condition fr.celEnv = opts.CELEnvironment } diff --git a/pkg/api/kptfile/v1/types.go b/pkg/api/kptfile/v1/types.go index f2559bba74..5e2f36c000 100644 --- a/pkg/api/kptfile/v1/types.go +++ b/pkg/api/kptfile/v1/types.go @@ -363,15 +363,16 @@ type Function struct { Exclusions []Selector `yaml:"exclude,omitempty" json:"exclude,omitempty"` // `Condition` is an optional CEL expression that determines whether this - // function should be executed. The expression is evaluated against the KRM - // resources in the package and should return a boolean value. + // function should be executed. The expression is evaluated against the list + // of KRM resources passed to this function step (after `Selectors` and + // `Exclude` have been applied) 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: + // + // 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: + // Example: Check resource count among the selected resources: // condition: "resources.filter(r, r.kind == 'Deployment').size() > 0" Condition string `yaml:"condition,omitempty" json:"condition,omitempty"` } diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index 5c7f5fce50..f88088e039 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -135,8 +135,22 @@ func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { if _, ok := result["kind"]; !ok { result["kind"] = "" } - if _, ok := result["metadata"]; !ok { - result["metadata"] = map[string]interface{}{} + // Ensure metadata and its common nested keys exist 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 { + result["metadata"] = map[string]interface{}{"name": "", "namespace": ""} + } + } else { + result["metadata"] = map[string]interface{}{"name": "", "namespace": ""} } return result, nil } From ffeadea0260f628000246846140603c725330f34 Mon Sep 17 00:00:00 2001 From: SurbhiAgarwal1 Date: Fri, 3 Apr 2026 00:27:28 +0530 Subject: [PATCH 16/19] fix: address PR review feedback from mozesl-nokia and Copilot - Remove unnecessary type alias file (internal/fnruntime/celeval.go) Use *runneroptions.CELEnvironment directly in runner.go - Group CEL constants in const() block in celenv.go - Replace interface{} with any throughout celenv.go for modern Go style - Remove panic from InitDefaults, add separate InitCELEnvironment() method Callers can now handle CEL initialization errors gracefully - Fix error message for exec-based functions when condition is set Now uses f.Exec as fallback when f.Image is empty - Fix go.mod: mark k8s.io/apiserver as direct dependency (not indirect) - Add InitCELEnvironment() calls after all InitDefaults() calls: * commands/fn/render/cmdrender.go * thirdparty/cmdconfig/commands/cmdeval/cmdeval.go * internal/util/get/get.go * pkg/lib/kptops/fs_test.go * pkg/lib/kptops/render_test.go Addresses review comments from: - mozesl-nokia (type alias, const grouping, panic removal, any vs interface{}) - Copilot (error messages, go.mod dependency marking) Signed-off-by: Surbhi --- PR_REVIEW_SUMMARY.md | 77 +++++++++++++++++++ commands/fn/render/cmdrender.go | 3 + e2e_output.txt | 12 +++ go.mod | 2 +- internal/fnruntime/celeval.go | 21 ----- internal/fnruntime/runner.go | 10 ++- internal/util/get/get.go | 3 + krm-functions-catalog | 1 + output.txt | 72 +++++++++++++++++ pkg/lib/kptops/fs_test.go | 6 ++ pkg/lib/kptops/render_test.go | 3 + pkg/lib/runneroptions/celenv.go | 25 +++--- pkg/lib/runneroptions/runneroptions.go | 11 ++- .../cmdconfig/commands/cmdeval/cmdeval.go | 3 + 14 files changed, 209 insertions(+), 40 deletions(-) create mode 100644 PR_REVIEW_SUMMARY.md create mode 100644 e2e_output.txt delete mode 100644 internal/fnruntime/celeval.go create mode 160000 krm-functions-catalog create mode 100644 output.txt diff --git a/PR_REVIEW_SUMMARY.md b/PR_REVIEW_SUMMARY.md new file mode 100644 index 0000000000..49dbf769e3 --- /dev/null +++ b/PR_REVIEW_SUMMARY.md @@ -0,0 +1,77 @@ +# PR #4391 Review Feedback Summary + +## Review from mozesl-nokia (6 hours ago) + +### 1. Remove unnecessary type alias file +**File:** `internal/fnruntime/celeval.go` +**Issue:** Having a file just for a type alias seems unnecessary +**Action:** Consider removing this file and using `*runneroptions.CELEnvironment` directly in `runner.go` + +### 2. Group constants together +**File:** `pkg/lib/runneroptions/celenv.go` (lines 28-31) +**Issue:** Constants should be grouped in a single `const ()` block +**Suggested change:** +```go +const ( + celCheckFrequency = 100 + // celCostLimit gives about .1 seconds of CPU time for the evaluation to run + celCostLimit = 1000000 +) +``` + +### 3. Avoid panic in InitDefaults +**File:** `pkg/lib/runneroptions/runneroptions.go` (lines 70-76) +**Issue:** `InitDefaults` panics on CEL environment initialization failure, which crashes the process +**Action:** Move CEL environment creation out of `InitDefaults` and let callers handle the error +**Recommendation:** Callers of `InitDefaults` should try to create CEL environment separately and return errors gracefully + +### 4. Use `any` instead of `interface{}` +**File:** `pkg/lib/runneroptions/celenv.go` +**Issue:** Modern Go style prefers `any` over `interface{}` +**Action:** Replace all `interface{}` with `any` + +## Copilot Review Comments (4 days ago) + +### 1. Better error message for exec-based functions +**File:** `internal/fnruntime/runner.go` (line 165) +**Issue:** Error uses `f.Image` which is empty for exec-based functions +**Suggested fix:** +```go +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) +``` + +### 2. Fix go.mod dependency +**File:** `go.mod` (line 136) +**Issue:** `k8s.io/apiserver v0.34.1 // indirect` should be a direct dependency since it's imported directly +**Action:** Remove `// indirect` comment - `go mod tidy` will fix this + +## Additional Context + +### From nagygergo (yesterday): +- Code and tests look good +- Documentation updates needed: + 1. Update https://kpt.dev/reference/schema/kptfile/ + 2. Add new chapter to https://kpt.dev/book/04-using-functions/ + +### Question to clarify: +Should documentation updates be part of this PR or a separate PR? + +## Files to Modify + +1. `internal/fnruntime/celeval.go` - Consider removing or justify keeping +2. `pkg/lib/runneroptions/celenv.go` - Group constants, use `any` instead of `interface{}` +3. `pkg/lib/runneroptions/runneroptions.go` - Remove panic from `InitDefaults` +4. `internal/fnruntime/runner.go` - Improve error message for exec functions +5. `go.mod` - Fix k8s.io/apiserver dependency marking + +## Next Steps + +1. Address all review comments from mozesl-nokia +2. Respond to or resolve Copilot comments +3. Run `go mod tidy` to fix dependency issues +4. Clarify documentation approach with maintainers +5. Test all changes locally before pushing diff --git a/commands/fn/render/cmdrender.go b/commands/fn/render/cmdrender.go index 43426ebef9..ceb1cc0b01 100644 --- a/commands/fn/render/cmdrender.go +++ b/commands/fn/render/cmdrender.go @@ -85,6 +85,9 @@ type Runner struct { func (r *Runner) InitDefaults() { r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix) + // Initialize CEL environment for condition evaluation + // Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime + _ = r.RunnerOptions.InitCELEnvironment() } func (r *Runner) preRunE(_ *cobra.Command, args []string) error { diff --git a/e2e_output.txt b/e2e_output.txt new file mode 100644 index 0000000000..85ba7a856d --- /dev/null +++ b/e2e_output.txt @@ -0,0 +1,12 @@ +=== RUN TestFnRender +=== RUN TestFnRender/testdata\fn-render\subpkg-has-samename-subdir +=== PAUSE TestFnRender/testdata\fn-render\subpkg-has-samename-subdir +=== CONT TestFnRender/testdata\fn-render\subpkg-has-samename-subdir + runner.go:79: failed to find kpt binary: cannot find command 'kpt' in $PATH: exec: "which": executable file not found in %PATH% + runner.go:297: Running test against package subpkg-has-samename-subdir, iteration 1 + fn_test.go:89: failed when running test: failed to copy package: exec: "cp": executable file not found in %PATH% +--- FAIL: TestFnRender (0.05s) + --- FAIL: TestFnRender/testdata\fn-render\subpkg-has-samename-subdir (0.01s) +FAIL +FAIL github.com/kptdev/kpt/e2e 0.359s +FAIL diff --git a/go.mod b/go.mod index 7972891169..431997e16a 100644 --- a/go.mod +++ b/go.mod @@ -133,7 +133,7 @@ require ( 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 + k8s.io/apiserver v0.34.1 k8s.io/component-helpers v0.34.1 // indirect k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect diff --git a/internal/fnruntime/celeval.go b/internal/fnruntime/celeval.go deleted file mode 100644 index e578bea5d1..0000000000 --- a/internal/fnruntime/celeval.go +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2026 The kpt and Nephio Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package fnruntime - -import "github.com/kptdev/kpt/pkg/lib/runneroptions" - -// CELEvaluator is a type alias for runneroptions.CELEnvironment, re-exported -// through the fnruntime package for convenience. -type CELEvaluator = runneroptions.CELEnvironment diff --git a/internal/fnruntime/runner.go b/internal/fnruntime/runner.go index f84c87121f..8723eb0cf0 100644 --- a/internal/fnruntime/runner.go +++ b/internal/fnruntime/runner.go @@ -162,7 +162,11 @@ func NewRunner( // 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) + 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) } fr.condition = f.Condition fr.celEnv = opts.CELEnvironment @@ -209,8 +213,8 @@ type FunctionRunner struct { fnResult *fnresult.Result fnResults *fnresult.ResultList opts runneroptions.RunnerOptions - condition string // CEL condition expression - celEnv *CELEvaluator // shared CEL environment for condition evaluation + condition string // CEL condition expression + celEnv *runneroptions.CELEnvironment // shared CEL environment for condition evaluation } func (fr *FunctionRunner) Filter(input []*yaml.RNode) (output []*yaml.RNode, err error) { diff --git a/internal/util/get/get.go b/internal/util/get/get.go index ae30f59896..cf0b25ea67 100644 --- a/internal/util/get/get.go +++ b/internal/util/get/get.go @@ -132,6 +132,9 @@ func (c Command) Run(ctx context.Context) error { pr.Printf("\nCustomizing package for deployment.\n") hookCmd := hook.Executor{} hookCmd.RunnerOptions.InitDefaults(c.DefaultKrmFunctionImagePrefix) + // Initialize CEL environment for condition evaluation + // Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime + _ = hookCmd.RunnerOptions.InitCELEnvironment() hookCmd.PkgPath = c.Destination builtinHooks := []kptfilev1.Function{ diff --git a/krm-functions-catalog b/krm-functions-catalog new file mode 160000 index 0000000000..0f35140a0d --- /dev/null +++ b/krm-functions-catalog @@ -0,0 +1 @@ +Subproject commit 0f35140a0dc8fe78bfb8562703295420ac1c8e24 diff --git a/output.txt b/output.txt new file mode 100644 index 0000000000..ff55726ad5 --- /dev/null +++ b/output.txt @@ -0,0 +1,72 @@ +=== RUN TestCmd_flagAndArgParsing_Symlink + cmdrender_test.go:35: + Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:35 + Error: Received unexpected error: + symlink path\to\pkg\dir foo: A required privilege is not held by the client. + Test: TestCmd_flagAndArgParsing_Symlink +Error: GetFileAttributesEx foo: The system cannot find the file specified. +Usage: + render [PKG_PATH] [flags] + +Examples: + + # Render the package in current directory + $ kpt fn render + + # Render the package in current directory and save results in my-results-dir + $ kpt fn render --results-dir my-results-dir + + # Render my-package-dir + $ kpt fn render my-package-dir + + # Render the package in current directory and write output resources to another DIR + $ kpt fn render -o path/to/dir + + # Render resources in current directory and write unwrapped resources to stdout + # which can be piped to kubectl apply + $ kpt fn render -o unwrap | kpt fn eval -i ghcr.io/kptdev/krm-functions-catalog/remove-local-config-resources:latest -o unwrap - | kubectl apply -f - + + # Render resources in current directory, write the wrapped resources + # to stdout which are piped to 'set-annotations' function, + # the transformed resources are written to another directory + $ kpt fn render -o stdout \ + | kpt fn eval - -i ghcr.io/kptdev/krm-functions-catalog/set-annotations:latest -o path/to/dir -- foo=bar + + # Render my-package-dir with podman as runtime for functions + $ KRM_FN_RUNTIME=podman kpt fn render my-package-dir + + # Render my-package-dir with network access enabled for functions + $ kpt fn render --allow-network + + +Flags: + --allow-alpha-wasm allow wasm to be used during pipeline execution. + --allow-exec allow binary executable to be run during pipeline execution. + --allow-network allow functions to access network during pipeline execution. + -h, --help help for render + --image-pull-policy ImagePullPolicy pull image before running the container (one of Always, IfNotPresent, Never) (default IfNotPresent) + -o, --output string output resources are written to provided location. Allowed values: stdout|unwrap| + --results-dir string path to a directory to save function results + + cmdrender_test.go:42: + Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:42 + Error: Received unexpected error: + GetFileAttributesEx foo: The system cannot find the file specified. + Test: TestCmd_flagAndArgParsing_Symlink + cmdrender_test.go:43: + Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:43 + Error: Not equal: + expected: "path\\to\\pkg\\dir" + actual : "" + + Diff: + --- Expected + +++ Actual + @@ -1 +1 @@ + -path\to\pkg\dir + + + Test: TestCmd_flagAndArgParsing_Symlink +--- FAIL: TestCmd_flagAndArgParsing_Symlink (0.02s) +FAIL +FAIL github.com/kptdev/kpt/commands/fn/render 0.494s +FAIL diff --git a/pkg/lib/kptops/fs_test.go b/pkg/lib/kptops/fs_test.go index d989f2fe15..9dcebea9b6 100644 --- a/pkg/lib/kptops/fs_test.go +++ b/pkg/lib/kptops/fs_test.go @@ -107,6 +107,9 @@ spec: Runtime: &runtime{}, } r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix) + if err := r.RunnerOptions.InitCELEnvironment(); err != nil { + t.Fatalf("Failed to initialize CEL environment: %v", err) + } r.RunnerOptions.ImagePullPolicy = runneroptions.IfNotPresentPull _, err := r.Execute(fake.CtxWithDefaultPrinter()) if err != nil { @@ -221,6 +224,9 @@ spec: Runtime: &runtime{}, } r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix) + if err := r.RunnerOptions.InitCELEnvironment(); err != nil { + t.Fatalf("Failed to initialize CEL environment: %v", err) + } r.RunnerOptions.ImagePullPolicy = runneroptions.IfNotPresentPull _, err := r.Execute(fake.CtxWithDefaultPrinter()) diff --git a/pkg/lib/kptops/render_test.go b/pkg/lib/kptops/render_test.go index 8170e236b7..de61adfecd 100644 --- a/pkg/lib/kptops/render_test.go +++ b/pkg/lib/kptops/render_test.go @@ -73,6 +73,9 @@ func TestRender(t *testing.T) { Output: &output, } r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix) + if err := r.RunnerOptions.InitCELEnvironment(); err != nil { + t.Fatalf("Failed to initialize CEL environment: %v", err) + } if _, err := r.Execute(fake.CtxWithDefaultPrinter()); err != nil { t.Errorf("Render failed: %v", err) diff --git a/pkg/lib/runneroptions/celenv.go b/pkg/lib/runneroptions/celenv.go index f88088e039..aeda0bc3b0 100644 --- a/pkg/lib/runneroptions/celenv.go +++ b/pkg/lib/runneroptions/celenv.go @@ -25,10 +25,11 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -const celCheckFrequency = 100 - -// celCostLimit gives about .1 seconds of CPU time for the evaluation to run -const celCostLimit = 1000000 +const ( + celCheckFrequency = 100 + // celCostLimit gives about .1 seconds of CPU time for the evaluation to run + celCostLimit = 1000000 +) // CELEnvironment holds a shared CEL environment for evaluating conditions. // The environment is created once and reused; programs are compiled per condition call. @@ -91,7 +92,7 @@ func (e *CELEnvironment) EvaluateCondition(ctx context.Context, condition string return false, fmt.Errorf("failed to convert resources: %w", err) } - out, _, err := prg.ContextEval(ctx, map[string]interface{}{ + out, _, err := prg.ContextEval(ctx, map[string]any{ "resources": resourceList, }) if err != nil { @@ -106,8 +107,8 @@ func (e *CELEnvironment) EvaluateCondition(ctx context.Context, condition string return bool(result), nil } -func resourcesToList(resources []*yaml.RNode) ([]interface{}, error) { - result := make([]interface{}, 0, len(resources)) +func resourcesToList(resources []*yaml.RNode) ([]any, error) { + result := make([]any, 0, len(resources)) for _, resource := range resources { m, err := resourceToMap(resource) if err != nil { @@ -118,12 +119,12 @@ func resourcesToList(resources []*yaml.RNode) ([]interface{}, error) { return result, nil } -func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { +func resourceToMap(resource *yaml.RNode) (map[string]any, error) { node := resource.YNode() if node == nil { return nil, fmt.Errorf("resource has nil yaml.Node") } - var result map[string]interface{} + var result map[string]any if err := node.Decode(&result); err != nil { return nil, fmt.Errorf("failed to decode resource: %w", err) } @@ -138,7 +139,7 @@ func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { // Ensure metadata and its common nested keys exist 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 mdMap, ok := mdVal.(map[string]any); ok { if _, ok := mdMap["name"]; !ok { mdMap["name"] = "" } @@ -147,10 +148,10 @@ func resourceToMap(resource *yaml.RNode) (map[string]interface{}, error) { } result["metadata"] = mdMap } else { - result["metadata"] = map[string]interface{}{"name": "", "namespace": ""} + result["metadata"] = map[string]any{"name": "", "namespace": ""} } } else { - result["metadata"] = map[string]interface{}{"name": "", "namespace": ""} + result["metadata"] = map[string]any{"name": "", "namespace": ""} } return result, nil } diff --git a/pkg/lib/runneroptions/runneroptions.go b/pkg/lib/runneroptions/runneroptions.go index 86f4f602bf..2fe3284004 100644 --- a/pkg/lib/runneroptions/runneroptions.go +++ b/pkg/lib/runneroptions/runneroptions.go @@ -67,13 +67,18 @@ type RunnerOptions struct { func (opts *RunnerOptions) InitDefaults(defaultImagePrefix string) { opts.ImagePullPolicy = IfNotPresentPull opts.ResolveToImage = opts.ResolveToImageForCLIFunc(defaultImagePrefix) +} + +// InitCELEnvironment initializes the CEL environment for condition evaluation. +// This should be called separately after InitDefaults to allow proper error handling. +// Returns an error if CEL environment creation fails. +func (opts *RunnerOptions) InitCELEnvironment() error { 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)) + return fmt.Errorf("failed to initialise CEL environment: %w", err) } opts.CELEnvironment = celEnv + return nil } // ResolveToImageForCLIFunc returns a func that converts the KRM function short path to the full image url. diff --git a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go index 5f0b9646cf..69d0b1557c 100644 --- a/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go +++ b/thirdparty/cmdconfig/commands/cmdeval/cmdeval.go @@ -166,6 +166,9 @@ type EvalFnRunner struct { func (r *EvalFnRunner) InitDefaults() { r.RunnerOptions.InitDefaults(runneroptions.GHCRImagePrefix) + // Initialize CEL environment for condition evaluation + // Ignore error as conditions are optional; if CEL init fails, conditions will error at runtime + _ = r.RunnerOptions.InitCELEnvironment() } func (r *EvalFnRunner) runE(c *cobra.Command, _ []string) error { From 367b250779b63fb67fdd761879e32a4fb7ce6bae Mon Sep 17 00:00:00 2001 From: SurbhiAgarwal1 Date: Fri, 3 Apr 2026 16:38:46 +0530 Subject: [PATCH 17/19] chore: trigger CI rebuild Signed-off-by: SurbhiAgarwal1 From 19bf57c9a6e5033c35746b9e7c79aeaaeb3de46f Mon Sep 17 00:00:00 2001 From: SurbhiAgarwal1 Date: Sat, 4 Apr 2026 01:26:56 +0530 Subject: [PATCH 18/19] chore: Remove unwanted files and fix test expectations - Remove krm-functions-catalog submodule (causing netlify docs build failure) - Remove temporary files: output.txt, e2e_output.txt, PR_REVIEW_SUMMARY.md - Update test expectations to include renderStatus field in Kptfile API - condition-not-met: empty mutationSteps array (function skipped) - condition-met: mutationSteps with exitCode 0 (function executed) Addresses review feedback from efiacor Signed-off-by: SurbhiAgarwal1 --- PR_REVIEW_SUMMARY.md | 77 ------------------- .../condition-met/.expected/diff.patch | 6 +- .../condition-not-met/.expected/diff.patch | 4 +- e2e_output.txt | 12 --- krm-functions-catalog | 1 - output.txt | 72 ----------------- 6 files changed, 8 insertions(+), 164 deletions(-) delete mode 100644 PR_REVIEW_SUMMARY.md delete mode 100644 e2e_output.txt delete mode 160000 krm-functions-catalog delete mode 100644 output.txt diff --git a/PR_REVIEW_SUMMARY.md b/PR_REVIEW_SUMMARY.md deleted file mode 100644 index 49dbf769e3..0000000000 --- a/PR_REVIEW_SUMMARY.md +++ /dev/null @@ -1,77 +0,0 @@ -# PR #4391 Review Feedback Summary - -## Review from mozesl-nokia (6 hours ago) - -### 1. Remove unnecessary type alias file -**File:** `internal/fnruntime/celeval.go` -**Issue:** Having a file just for a type alias seems unnecessary -**Action:** Consider removing this file and using `*runneroptions.CELEnvironment` directly in `runner.go` - -### 2. Group constants together -**File:** `pkg/lib/runneroptions/celenv.go` (lines 28-31) -**Issue:** Constants should be grouped in a single `const ()` block -**Suggested change:** -```go -const ( - celCheckFrequency = 100 - // celCostLimit gives about .1 seconds of CPU time for the evaluation to run - celCostLimit = 1000000 -) -``` - -### 3. Avoid panic in InitDefaults -**File:** `pkg/lib/runneroptions/runneroptions.go` (lines 70-76) -**Issue:** `InitDefaults` panics on CEL environment initialization failure, which crashes the process -**Action:** Move CEL environment creation out of `InitDefaults` and let callers handle the error -**Recommendation:** Callers of `InitDefaults` should try to create CEL environment separately and return errors gracefully - -### 4. Use `any` instead of `interface{}` -**File:** `pkg/lib/runneroptions/celenv.go` -**Issue:** Modern Go style prefers `any` over `interface{}` -**Action:** Replace all `interface{}` with `any` - -## Copilot Review Comments (4 days ago) - -### 1. Better error message for exec-based functions -**File:** `internal/fnruntime/runner.go` (line 165) -**Issue:** Error uses `f.Image` which is empty for exec-based functions -**Suggested fix:** -```go -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) -``` - -### 2. Fix go.mod dependency -**File:** `go.mod` (line 136) -**Issue:** `k8s.io/apiserver v0.34.1 // indirect` should be a direct dependency since it's imported directly -**Action:** Remove `// indirect` comment - `go mod tidy` will fix this - -## Additional Context - -### From nagygergo (yesterday): -- Code and tests look good -- Documentation updates needed: - 1. Update https://kpt.dev/reference/schema/kptfile/ - 2. Add new chapter to https://kpt.dev/book/04-using-functions/ - -### Question to clarify: -Should documentation updates be part of this PR or a separate PR? - -## Files to Modify - -1. `internal/fnruntime/celeval.go` - Consider removing or justify keeping -2. `pkg/lib/runneroptions/celenv.go` - Group constants, use `any` instead of `interface{}` -3. `pkg/lib/runneroptions/runneroptions.go` - Remove panic from `InitDefaults` -4. `internal/fnruntime/runner.go` - Improve error message for exec functions -5. `go.mod` - Fix k8s.io/apiserver dependency marking - -## Next Steps - -1. Address all review comments from mozesl-nokia -2. Respond to or resolve Copilot comments -3. Run `go mod tidy` to fix dependency issues -4. Clarify documentation approach with maintainers -5. Test all changes locally before pushing diff --git a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch index f24df5b132..28e8b8b064 100644 --- a/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-met/.expected/diff.patch @@ -2,7 +2,7 @@ diff --git a/Kptfile b/Kptfile index eb90ac3..ace574a 100644 --- a/Kptfile +++ b/Kptfile -@@ -5,4 +5,9 @@ metadata: +@@ -5,4 +5,12 @@ metadata: pipeline: mutators: - image: ghcr.io/kptdev/krm-functions-catalog/no-op @@ -13,3 +13,7 @@ index eb90ac3..ace574a 100644 + - type: Rendered + status: "True" + reason: RenderSuccess ++ renderStatus: ++ mutationSteps: ++ - image: ghcr.io/kptdev/krm-functions-catalog/no-op ++ exitCode: 0 diff --git a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch index f24df5b132..f569330fc0 100644 --- a/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch +++ b/e2e/testdata/fn-render/condition/condition-not-met/.expected/diff.patch @@ -2,7 +2,7 @@ diff --git a/Kptfile b/Kptfile index eb90ac3..ace574a 100644 --- a/Kptfile +++ b/Kptfile -@@ -5,4 +5,9 @@ metadata: +@@ -5,4 +5,11 @@ metadata: pipeline: mutators: - image: ghcr.io/kptdev/krm-functions-catalog/no-op @@ -13,3 +13,5 @@ index eb90ac3..ace574a 100644 + - type: Rendered + status: "True" + reason: RenderSuccess ++ renderStatus: ++ mutationSteps: [] diff --git a/e2e_output.txt b/e2e_output.txt deleted file mode 100644 index 85ba7a856d..0000000000 --- a/e2e_output.txt +++ /dev/null @@ -1,12 +0,0 @@ -=== RUN TestFnRender -=== RUN TestFnRender/testdata\fn-render\subpkg-has-samename-subdir -=== PAUSE TestFnRender/testdata\fn-render\subpkg-has-samename-subdir -=== CONT TestFnRender/testdata\fn-render\subpkg-has-samename-subdir - runner.go:79: failed to find kpt binary: cannot find command 'kpt' in $PATH: exec: "which": executable file not found in %PATH% - runner.go:297: Running test against package subpkg-has-samename-subdir, iteration 1 - fn_test.go:89: failed when running test: failed to copy package: exec: "cp": executable file not found in %PATH% ---- FAIL: TestFnRender (0.05s) - --- FAIL: TestFnRender/testdata\fn-render\subpkg-has-samename-subdir (0.01s) -FAIL -FAIL github.com/kptdev/kpt/e2e 0.359s -FAIL diff --git a/krm-functions-catalog b/krm-functions-catalog deleted file mode 160000 index 0f35140a0d..0000000000 --- a/krm-functions-catalog +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 0f35140a0dc8fe78bfb8562703295420ac1c8e24 diff --git a/output.txt b/output.txt deleted file mode 100644 index ff55726ad5..0000000000 --- a/output.txt +++ /dev/null @@ -1,72 +0,0 @@ -=== RUN TestCmd_flagAndArgParsing_Symlink - cmdrender_test.go:35: - Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:35 - Error: Received unexpected error: - symlink path\to\pkg\dir foo: A required privilege is not held by the client. - Test: TestCmd_flagAndArgParsing_Symlink -Error: GetFileAttributesEx foo: The system cannot find the file specified. -Usage: - render [PKG_PATH] [flags] - -Examples: - - # Render the package in current directory - $ kpt fn render - - # Render the package in current directory and save results in my-results-dir - $ kpt fn render --results-dir my-results-dir - - # Render my-package-dir - $ kpt fn render my-package-dir - - # Render the package in current directory and write output resources to another DIR - $ kpt fn render -o path/to/dir - - # Render resources in current directory and write unwrapped resources to stdout - # which can be piped to kubectl apply - $ kpt fn render -o unwrap | kpt fn eval -i ghcr.io/kptdev/krm-functions-catalog/remove-local-config-resources:latest -o unwrap - | kubectl apply -f - - - # Render resources in current directory, write the wrapped resources - # to stdout which are piped to 'set-annotations' function, - # the transformed resources are written to another directory - $ kpt fn render -o stdout \ - | kpt fn eval - -i ghcr.io/kptdev/krm-functions-catalog/set-annotations:latest -o path/to/dir -- foo=bar - - # Render my-package-dir with podman as runtime for functions - $ KRM_FN_RUNTIME=podman kpt fn render my-package-dir - - # Render my-package-dir with network access enabled for functions - $ kpt fn render --allow-network - - -Flags: - --allow-alpha-wasm allow wasm to be used during pipeline execution. - --allow-exec allow binary executable to be run during pipeline execution. - --allow-network allow functions to access network during pipeline execution. - -h, --help help for render - --image-pull-policy ImagePullPolicy pull image before running the container (one of Always, IfNotPresent, Never) (default IfNotPresent) - -o, --output string output resources are written to provided location. Allowed values: stdout|unwrap| - --results-dir string path to a directory to save function results - - cmdrender_test.go:42: - Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:42 - Error: Received unexpected error: - GetFileAttributesEx foo: The system cannot find the file specified. - Test: TestCmd_flagAndArgParsing_Symlink - cmdrender_test.go:43: - Error Trace: C:/Users/Surbhi/Catroid/kpt/commands/fn/render/cmdrender_test.go:43 - Error: Not equal: - expected: "path\\to\\pkg\\dir" - actual : "" - - Diff: - --- Expected - +++ Actual - @@ -1 +1 @@ - -path\to\pkg\dir - + - Test: TestCmd_flagAndArgParsing_Symlink ---- FAIL: TestCmd_flagAndArgParsing_Symlink (0.02s) -FAIL -FAIL github.com/kptdev/kpt/commands/fn/render 0.494s -FAIL From b6f3d8ee1b6a31fcf84d7ed23aa8a29ff330d884 Mon Sep 17 00:00:00 2001 From: SurbhiAgarwal1 Date: Sat, 4 Apr 2026 01:51:36 +0530 Subject: [PATCH 19/19] docs: add condition field documentation for CEL-based conditional function execution - Add condition section to book/04-using-functions with examples and CEL patterns - Update kptfile schema reference to include the condition field Signed-off-by: SurbhiAgarwal1 --- .../en/book/04-using-functions/_index.md | 62 +++++++++++++++++++ .../en/reference/schema/kptfile/kptfile.yaml | 10 +++ 2 files changed, 72 insertions(+) diff --git a/documentation/content/en/book/04-using-functions/_index.md b/documentation/content/en/book/04-using-functions/_index.md index 75ca80115f..9d53926f2d 100644 --- a/documentation/content/en/book/04-using-functions/_index.md +++ b/documentation/content/en/book/04-using-functions/_index.md @@ -375,6 +375,68 @@ will merge each function pipeline list as an associative list, using `name` as the merge key. An unspecified `name` or duplicated names may result in unexpected merges. +### Specifying `condition` + +The `condition` field lets you skip a function based on the current state of the resources in the package. +It takes a [CEL](https://cel.dev/) expression that is evaluated against the resource list. If the expression +returns `true`, the function runs. If it returns `false`, the function is skipped. + +The expression receives a variable called `resources`, which is a list of all KRM resources passed to +this function step (after `selectors` and `exclude` have been applied). Each resource is a map with +the standard fields: `apiVersion`, `kind`, `metadata`, `spec`, `status`. + +For example, only run the `set-labels` function if a `ConfigMap` named `app-config` exists in the package: + +```yaml +# wordpress/Kptfile (Excerpt) +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: wordpress +pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/set-labels:latest + configMap: + app: wordpress + condition: resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'app-config') +``` + +When you render the package, kpt shows whether the function ran or was skipped: + +```shell +$ kpt fn render wordpress +Package "wordpress": + +[RUNNING] "ghcr.io/kptdev/krm-functions-catalog/set-labels:latest" +[PASS] "ghcr.io/kptdev/krm-functions-catalog/set-labels:latest" + +Successfully executed 1 function(s) in 1 package(s). +``` + +If the condition is not met: + +```shell +$ kpt fn render wordpress +Package "wordpress": + +[SKIPPED] "ghcr.io/kptdev/krm-functions-catalog/set-labels:latest" (condition not met) + +Successfully executed 1 function(s) in 1 package(s). +``` + +Some useful CEL expression patterns: + +- Check if a resource of a specific kind exists: + `resources.exists(r, r.kind == 'Deployment')` +- Check if a specific resource exists by name: + `resources.exists(r, r.kind == 'ConfigMap' && r.metadata.name == 'my-config')` +- Check the count of resources: + `resources.filter(r, r.kind == 'Deployment').size() > 0` + +The `condition` field can be combined with `selectors` and `exclude`. The condition is evaluated +after selectors and exclusions are applied, so `resources` only contains the resources that +passed the selection criteria. + ### Specifying `selectors` In some cases, you want to invoke the function only on a subset of resources based on a diff --git a/documentation/content/en/reference/schema/kptfile/kptfile.yaml b/documentation/content/en/reference/schema/kptfile/kptfile.yaml index e13c8fc233..86e3820612 100644 --- a/documentation/content/en/reference/schema/kptfile/kptfile.yaml +++ b/documentation/content/en/reference/schema/kptfile/kptfile.yaml @@ -71,6 +71,16 @@ definitions: this is primarily used for merging function declaration with upstream counterparts type: string x-go-name: Name + condition: + description: |- + `Condition` is an optional CEL expression that determines whether this + function should be executed. The expression is evaluated against the list + of KRM resources passed to this function step (after `Selectors` and + `Exclude` have been applied) and should return a boolean value. + If omitted or evaluates to true, the function executes normally. + If evaluates to false, the function is skipped. + type: string + x-go-name: Condition selectors: description: |- `Selectors` are used to specify resources on which the function should be executed