Merge pull request #135759 from Abhigyan-Shekhar/fix-cel-race-condition

FIX: Deep copy MapType in CEL composition to prevent data race

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/composition.go

@@ -34,7 +34,7 @@ import (
 	"k8s.io/apiserver/pkg/cel/lazy"
 )
 
-const VariablesTypeName = "kubernetes.variables"
+const variablesTypeName = "kubernetes.variables"
 
 // CompositedCompiler compiles expressions with variable composition.
 type CompositedCompiler struct {
@@ -42,7 +42,7 @@ type CompositedCompiler struct {
 	ConditionCompiler
 	MutatingCompiler
 
-	CompositionEnv *CompositionEnv
+	state *compositionState
 }
 
 // CompositedConditionEvaluator provides evaluation of a condition expression with variable composition.
@@ -50,7 +50,7 @@ type CompositedCompiler struct {
 type CompositedConditionEvaluator struct {
 	ConditionEvaluator
 
-	compositionEnv *CompositionEnv
+	state *compositionState
 }
 
 // CompositedEvaluator provides evaluation of a single expression with variable composition.
@@ -58,32 +58,78 @@ type CompositedConditionEvaluator struct {
 type CompositedEvaluator struct {
 	MutatingEvaluator
 
-	compositionEnv *CompositionEnv
+	state *compositionState
 }
 
 func NewCompositedCompiler(envSet *environment.EnvSet) (*CompositedCompiler, error) {
-	compositionContext, err := NewCompositionEnv(VariablesTypeName, envSet)
+	newMapType := apiservercel.NewObjectType(variablesTypeName, map[string]*apiservercel.DeclField{})
+
+	newEnvSet, err := envSet.Extend(environment.VersionedOptions{
+		IntroducedVersion: version.MajorMinor(1, 0),
+		EnvOptions: []cel.EnvOption{
+			cel.Variable("variables", newMapType.CelType()),
+		},
+		DeclTypes: []*apiservercel.DeclType{
+			newMapType,
+		},
+	})
 	if err != nil {
 		return nil, err
 	}
-	return NewCompositedCompilerFromTemplate(compositionContext), nil
-}
 
-func NewCompositedCompilerFromTemplate(context *CompositionEnv) *CompositedCompiler {
-	context = &CompositionEnv{
-		MapType:           context.MapType,
-		EnvSet:            context.EnvSet,
-		CompiledVariables: map[string]CompilationResult{},
+	state := &compositionState{
+		mapType:           newMapType,
+		EnvSet:            newEnvSet,
+		compiledVariables: map[string]CompilationResult{},
 	}
-	compiler := NewCompiler(context.EnvSet)
+
+	compiler := NewCompiler(state.EnvSet)
 	conditionCompiler := &conditionCompiler{compiler}
 	mutation := &mutatingCompiler{compiler}
 	return &CompositedCompiler{
 		Compiler:          compiler,
 		ConditionCompiler: conditionCompiler,
 		MutatingCompiler:  mutation,
-		CompositionEnv:    context,
+		state:             state,
+	}, nil
+}
+
+// NewCompositedCompilerForTypeChecking creates a CompositedCompiler for type checking.
+// It initializes the composition state but leaves the Compilers nil, as they are expected
+// to be replaced by the caller (who is doing type checking).
+func NewCompositedCompilerForTypeChecking(envSet *environment.EnvSet) (*CompositedCompiler, error) {
+	newMapType := apiservercel.NewObjectType(variablesTypeName, map[string]*apiservercel.DeclField{})
+
+	newEnvSet, err := envSet.Extend(environment.VersionedOptions{
+		IntroducedVersion: version.MajorMinor(1, 0),
+		EnvOptions: []cel.EnvOption{
+			cel.Variable("variables", newMapType.CelType()),
+		},
+		DeclTypes: []*apiservercel.DeclType{
+			newMapType,
+		},
+	})
+	if err != nil {
+		return nil, err
 	}
+
+	state := &compositionState{
+		mapType:           newMapType,
+		EnvSet:            newEnvSet,
+		compiledVariables: map[string]CompilationResult{},
+	}
+	return &CompositedCompiler{
+		state: state,
+	}, nil
+}
+
+// Env returns the CEL environment for the given mode.
+func (c *CompositedCompiler) Env(mode environment.Type) (*cel.Env, error) {
+	return c.state.Env(mode)
+}
+
+func (c *CompositedCompiler) CreateContext(parent context.Context) CompositionContext {
+	return c.state.CreateContext(parent)
 }
 
 func (c *CompositedCompiler) CompileAndStoreVariables(variables []NamedExpressionAccessor, options OptionalVariableDeclarations, mode environment.Type) {
@@ -94,8 +140,8 @@ func (c *CompositedCompiler) CompileAndStoreVariables(variables []NamedExpressio
 
 func (c *CompositedCompiler) CompileAndStoreVariable(variable NamedExpressionAccessor, options OptionalVariableDeclarations, mode environment.Type) CompilationResult {
 	result := c.Compiler.CompileCELExpression(variable, options, mode)
-	c.CompositionEnv.AddField(variable.GetName(), result.OutputType)
-	c.CompositionEnv.CompiledVariables[variable.GetName()] = result
+	c.state.AddField(variable.GetName(), result.OutputType)
+	c.state.compiledVariables[variable.GetName()] = result
 	return result
 }
 
@@ -103,7 +149,7 @@ func (c *CompositedCompiler) CompileCondition(expressions []ExpressionAccessor,
 	condition := c.ConditionCompiler.CompileCondition(expressions, optionalDecls, envType)
 	return &CompositedConditionEvaluator{
 		ConditionEvaluator: condition,
-		compositionEnv:     c.CompositionEnv,
+		state:              c.state,
 	}
 }
 
@@ -112,47 +158,25 @@ func (c *CompositedCompiler) CompileMutatingEvaluator(expression ExpressionAcces
 	mutation := c.MutatingCompiler.CompileMutatingEvaluator(expression, optionalDecls, envType)
 	return &CompositedEvaluator{
 		MutatingEvaluator: mutation,
-		compositionEnv:    c.CompositionEnv,
+		state:             c.state,
 	}
 }
 
-type CompositionEnv struct {
+type compositionState struct {
 	*environment.EnvSet
 
-	MapType           *apiservercel.DeclType
-	CompiledVariables map[string]CompilationResult
+	mapType           *apiservercel.DeclType
+	compiledVariables map[string]CompilationResult
 }
 
-func (c *CompositionEnv) AddField(name string, celType *cel.Type) {
-	c.MapType.Fields[name] = apiservercel.NewDeclField(name, convertCelTypeToDeclType(celType), true, nil, nil)
+func (c *compositionState) AddField(name string, celType *cel.Type) {
+	c.mapType.Fields[name] = apiservercel.NewDeclField(name, convertCelTypeToDeclType(celType), true, nil, nil)
 }
 
-func NewCompositionEnv(typeName string, baseEnvSet *environment.EnvSet) (*CompositionEnv, error) {
-	declType := apiservercel.NewObjectType(typeName, map[string]*apiservercel.DeclField{})
-	envSet, err := baseEnvSet.Extend(environment.VersionedOptions{
-		// set to 1.0 because composition is one of the fundamental components
-		IntroducedVersion: version.MajorMinor(1, 0),
-		EnvOptions: []cel.EnvOption{
-			cel.Variable("variables", declType.CelType()),
-		},
-		DeclTypes: []*apiservercel.DeclType{
-			declType,
-		},
-	})
-	if err != nil {
-		return nil, err
-	}
-	return &CompositionEnv{
-		MapType:           declType,
-		EnvSet:            envSet,
-		CompiledVariables: map[string]CompilationResult{},
-	}, nil
-}
-
-func (c *CompositionEnv) CreateContext(parent context.Context) CompositionContext {
+func (c *compositionState) CreateContext(parent context.Context) CompositionContext {
 	return &compositionContext{
-		Context:        parent,
-		compositionEnv: c,
+		Context: parent,
+		state:   c,
 	}
 }
 
@@ -165,13 +189,13 @@ type CompositionContext interface {
 type compositionContext struct {
 	context.Context
 
-	compositionEnv  *CompositionEnv
+	state           *compositionState
 	accumulatedCost int64
 }
 
 func (c *compositionContext) Variables(activation any) ref.Val {
-	lazyMap := lazy.NewMapValue(c.compositionEnv.MapType)
-	for name, result := range c.compositionEnv.CompiledVariables {
+	lazyMap := lazy.NewMapValue(c.state.mapType)
+	for name, result := range c.state.compiledVariables {
 		accessor := &variableAccessor{
 			name:       name,
 			result:     result,
@@ -184,7 +208,7 @@ func (c *compositionContext) Variables(activation any) ref.Val {
 }
 
 func (f *CompositedConditionEvaluator) ForInput(ctx context.Context, versionedAttr *admission.VersionedAttributes, request *v1.AdmissionRequest, optionalVars OptionalVariableBindings, namespace *corev1.Namespace, runtimeCELCostBudget int64) ([]EvaluationResult, int64, error) {
-	ctx = f.compositionEnv.CreateContext(ctx)
+	ctx = f.state.CreateContext(ctx)
 	return f.ConditionEvaluator.ForInput(ctx, versionedAttr, request, optionalVars, namespace, runtimeCELCostBudget)
 }
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/composition_test.go

@@ -247,3 +247,46 @@ func TestCompositedPolicies(t *testing.T) {
 		})
 	}
 }
+
+// TestCompilerIsolation verifies that each call to NewCompositedCompiler
+// creates an isolated environment where variables from one compiler
+// do not leak into another.
+func TestCompilerIsolation(t *testing.T) {
+	baseEnv := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())
+
+	// Create first compiler with variable "foo"
+	compiler1, err := NewCompositedCompiler(baseEnv)
+	if err != nil {
+		t.Fatal(err)
+	}
+	vars1 := []NamedExpressionAccessor{
+		&testVariable{name: "foo", expression: "'bar'"},
+	}
+	compiler1.CompileAndStoreVariables(vars1, OptionalVariableDeclarations{}, environment.StoredExpressions)
+
+	// Create second compiler with variable "baz"
+	compiler2, err := NewCompositedCompiler(baseEnv)
+	if err != nil {
+		t.Fatal(err)
+	}
+	vars2 := []NamedExpressionAccessor{
+		&testVariable{name: "baz", expression: "'qux'"},
+	}
+	compiler2.CompileAndStoreVariables(vars2, OptionalVariableDeclarations{}, environment.StoredExpressions)
+
+	// Verify isolation: compiler1 should not have "baz", compiler2 should not have "foo"
+	if _, ok := compiler1.state.mapType.Fields["baz"]; ok {
+		t.Error("compiler1 should not have variable 'baz' from compiler2")
+	}
+	if _, ok := compiler2.state.mapType.Fields["foo"]; ok {
+		t.Error("compiler2 should not have variable 'foo' from compiler1")
+	}
+
+	// Verify each compiler has its own variable
+	if _, ok := compiler1.state.mapType.Fields["foo"]; !ok {
+		t.Error("compiler1 should have variable 'foo'")
+	}
+	if _, ok := compiler2.state.mapType.Fields["baz"]; !ok {
+		t.Error("compiler2 should have variable 'baz'")
+	}
+}

staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/condition_test.go

@@ -803,7 +803,7 @@ func TestCondition(t *testing.T) {
 			attributes: newValidAttribute(nil, false),
 			results: []EvaluationResult{
 				{
-					Error: errors.New(fmt.Sprintf("operation cancelled: actual cost limit exceeded")),
+					Error: fmt.Errorf("operation cancelled: actual cost limit exceeded"),
 				},
 			},
 			hasParamKind:     true,
@@ -1573,6 +1573,8 @@ func (f fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes)
 			panic(fmt.Sprintf("unsupported type: %T", a))
 		}
 
+		// Compare AttributesRecord structs - contains error fields but we're comparing the struct, not handling errors
+		//nolint:all
 		if reflect.DeepEqual(f.match.match, *other) {
 			return f.match.decision, f.match.reason, f.match.err
 		}

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation.go

@@ -77,5 +77,5 @@ func compilePolicy(policy *Policy) PolicyEvaluator {
 		}
 	}
 
-	return PolicyEvaluator{Matcher: matcher, Mutators: patchers, CompositionEnv: compiler.CompositionEnv}
+	return PolicyEvaluator{Matcher: matcher, Mutators: patchers, CompositedCompiler: compiler}
 }

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation_test.go

@@ -18,11 +18,12 @@ package mutating
 
 import (
 	"context"
-	"github.com/google/go-cmp/cmp"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+
 	"k8s.io/api/admissionregistration/v1beta1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -326,8 +327,8 @@ func TestCompilation(t *testing.T) {
 			}
 
 			policyEvaluator := compilePolicy(tc.policy)
-			if policyEvaluator.CompositionEnv != nil {
-				ctx = policyEvaluator.CompositionEnv.CreateContext(ctx)
+			if policyEvaluator.CompositedCompiler != nil {
+				ctx = policyEvaluator.CompositedCompiler.CreateContext(ctx)
 			}
 			obj := tc.object
 

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go

@@ -49,7 +49,7 @@ func NewDispatcher(a authorizer.Authorizer, m *matching.Matcher, tcm patch.TypeC
 		authz:                a,
 		typeConverterManager: tcm,
 	}
-	res.Dispatcher = generic.NewPolicyDispatcher[*Policy, *PolicyBinding, PolicyEvaluator](
+	res.Dispatcher = generic.NewPolicyDispatcher(
 		NewMutatingAdmissionPolicyAccessor,
 		NewMutatingAdmissionPolicyBindingAccessor,
 		m,
@@ -137,8 +137,8 @@ func (d *dispatcher) dispatchInvocations(
 	// Should loop through invocations, handling possible error and invoking
 	// evaluator to apply patch, also should handle re-invocations
 	for _, invocation := range invocations {
-		if invocation.Evaluator.CompositionEnv != nil {
-			ctx = invocation.Evaluator.CompositionEnv.CreateContext(ctx)
+		if invocation.Evaluator.CompositedCompiler != nil {
+			ctx = invocation.Evaluator.CompositedCompiler.CreateContext(ctx)
 		}
 		if len(invocation.Evaluator.Mutators) != len(invocation.Policy.Spec.Mutations) {
 			// This would be a bug. The compiler should always return exactly as

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin.go

@@ -18,9 +18,10 @@ package mutating
 
 import (
 	"context"
-	celgo "github.com/google/cel-go/cel"
 	"io"
 
+	celgo "github.com/google/cel-go/cel"
+
 	"k8s.io/api/admissionregistration/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -73,10 +74,10 @@ type MutationEvaluationFunc func(
 ) (runtime.Object, error)
 
 type PolicyEvaluator struct {
-	Matcher        matchconditions.Matcher
-	Mutators       []patch.Patcher
-	CompositionEnv *cel.CompositionEnv
-	Error          error
+	Matcher            matchconditions.Matcher
+	Mutators           []patch.Patcher
+	CompositedCompiler *cel.CompositedCompiler
+	Error              error
 }
 
 // Plugin is an implementation of admission.Interface.

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/plugin.go

@@ -43,16 +43,12 @@ const (
 
 var (
 	lazyCompositionEnvTemplateWithStrictCostInit sync.Once
-	lazyCompositionEnvTemplateWithStrictCost     *cel.CompositionEnv
+	lazyCompositionEnvTemplateWithStrictCost     *environment.EnvSet
 )
 
-func getCompositionEnvTemplateWithStrictCost() *cel.CompositionEnv {
+func getCompositionEnvTemplateWithStrictCost() *environment.EnvSet {
 	lazyCompositionEnvTemplateWithStrictCostInit.Do(func() {
-		env, err := cel.NewCompositionEnv(cel.VariablesTypeName, environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
-		if err != nil {
-			panic(err)
-		}
-		lazyCompositionEnvTemplateWithStrictCost = env
+		lazyCompositionEnvTemplateWithStrictCost = environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())
 	})
 	return lazyCompositionEnvTemplateWithStrictCost
 }
@@ -120,9 +116,11 @@ func compilePolicy(policy *Policy) Validator {
 	failurePolicy := policy.Spec.FailurePolicy
 	var matcher matchconditions.Matcher = nil
 	matchConditions := policy.Spec.MatchConditions
-	var compositionEnvTemplate *cel.CompositionEnv
-	compositionEnvTemplate = getCompositionEnvTemplateWithStrictCost()
-	filterCompiler := cel.NewCompositedCompilerFromTemplate(compositionEnvTemplate)
+	compositionEnvTemplate := getCompositionEnvTemplateWithStrictCost()
+	filterCompiler, err := cel.NewCompositedCompiler(compositionEnvTemplate)
+	if err != nil {
+		return NewValidator(nil, nil, nil, nil, failurePolicy, err)
+	}
 	filterCompiler.CompileAndStoreVariables(convertv1beta1Variables(policy.Spec.Variables), optionalVars, environment.StoredExpressions)
 
 	if len(matchConditions) > 0 {
@@ -138,6 +136,7 @@ func compilePolicy(policy *Policy) Validator {
 		filterCompiler.CompileCondition(convertv1AuditAnnotations(policy.Spec.AuditAnnotations), optionalVars, environment.StoredExpressions),
 		filterCompiler.CompileCondition(convertv1MessageExpressions(policy.Spec.Validations), expressionOptionalVars, environment.StoredExpressions),
 		failurePolicy,
+		nil,
 	)
 
 	return res

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/typechecking.go

@@ -183,14 +183,11 @@ func (c *TypeChecker) compiler(ctx *TypeCheckingContext, typeOverwrite typeOverw
 	if err != nil {
 		return nil, err
 	}
-	env, err := plugincel.NewCompositionEnv(plugincel.VariablesTypeName, envSet)
+	compiler, err := plugincel.NewCompositedCompilerForTypeChecking(envSet)
 	if err != nil {
 		return nil, err
 	}
-	compiler := &plugincel.CompositedCompiler{
-		Compiler:       &typeCheckingCompiler{typeOverwrite: typeOverwrite, compositionEnv: env},
-		CompositionEnv: env,
-	}
+	compiler.Compiler = &typeCheckingCompiler{typeOverwrite: typeOverwrite, compiler: compiler}
 	return compiler, nil
 }
 
@@ -449,8 +446,8 @@ func createVariableOpts(declType *apiservercel.DeclType, variables ...string) []
 }
 
 type typeCheckingCompiler struct {
-	compositionEnv *plugincel.CompositionEnv
-	typeOverwrite  typeOverwrite
+	compiler      *plugincel.CompositedCompiler
+	typeOverwrite typeOverwrite
 }
 
 // CompileCELExpression compiles the given expression.
@@ -469,7 +466,7 @@ func (c *typeCheckingCompiler) CompileCELExpression(expressionAccessor plugincel
 			ExpressionAccessor: expressionAccessor,
 		}
 	}
-	env, err := c.compositionEnv.Env(mode)
+	env, err := c.compiler.Env(mode)
 	if err != nil {
 		return resultError(fmt.Sprintf("fail to build env: %v", err), apiservercel.ErrorTypeInternal)
 	}

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go

@@ -45,15 +45,19 @@ type validator struct {
 	auditAnnotationFilter cel.ConditionEvaluator
 	messageFilter         cel.ConditionEvaluator
 	failPolicy            *v1.FailurePolicyType
+	// compileError holds any compilation error from the CEL expressions.
+	// If non-nil, the validator will return an error result based on the failPolicy.
+	compileError error
 }
 
-func NewValidator(validationFilter cel.ConditionEvaluator, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.ConditionEvaluator, failPolicy *v1.FailurePolicyType) Validator {
+func NewValidator(validationFilter cel.ConditionEvaluator, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.ConditionEvaluator, failPolicy *v1.FailurePolicyType, err error) Validator {
 	return &validator{
 		celMatcher:            celMatcher,
 		validationFilter:      validationFilter,
 		auditAnnotationFilter: auditAnnotationFilter,
 		messageFilter:         messageFilter,
 		failPolicy:            failPolicy,
+		compileError:          err,
 	}
 }
 
@@ -81,6 +85,17 @@ func (v *validator) Validate(ctx context.Context, matchedResource schema.GroupVe
 	} else {
 		f = *v.failPolicy
 	}
+	if v.compileError != nil {
+		return ValidateResult{
+			Decisions: []PolicyDecision{
+				{
+					Action:     policyDecisionActionForError(f),
+					Evaluation: EvalError,
+					Message:    v.compileError.Error(),
+				},
+			},
+		}
+	}
 	if v.celMatcher != nil {
 		matchResults := v.celMatcher.Match(ctx, versionedAttr, versionedParams, authz)
 		if matchResults.Error != nil {

staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go

@@ -53,7 +53,7 @@ func (f *fakeCelFilter) ForInput(ctx context.Context, versionedAttr *admission.V
 	if costBudget <= 0 { // this filter will cost 1, so cost = 0 means fail.
 		return nil, -1, &apiservercel.Error{
 			Type:   apiservercel.ErrorTypeInvalid,
-			Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"),
+			Detail: "validation failed due to running out of cost budget, no further validation rules will be run",
 			Cause:  apiservercel.ErrOutOfBudget,
 		}
 	}

staging/src/k8s.io/kubectl/pkg/util/resource/resource.go

@@ -255,7 +255,7 @@ func convertResourceEphemeralStorageToString(ephemeralStorage *resource.Quantity
 	return strconv.FormatInt(m, 10), nil
 }
 
-var standardContainerResources = sets.New[string](
+var standardContainerResources = sets.New(
 	string(corev1.ResourceCPU),
 	string(corev1.ResourceMemory),
 	string(corev1.ResourceEphemeralStorage),