diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking.go index 86c8479c3a4..d4a1bf82556 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking.go @@ -28,6 +28,7 @@ import ( "k8s.io/api/admissionregistration/v1beta1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/version" @@ -55,6 +56,8 @@ type TypeCheckingContext struct { declTypes []*apiservercel.DeclType paramGVK schema.GroupVersionKind paramDeclType *apiservercel.DeclType + + variables []v1beta1.Variable } type typeOverwrite struct { @@ -68,7 +71,7 @@ type TypeCheckingResult struct { // GVK is the associated GVK GVK schema.GroupVersionKind // Issues contain machine-readable information about the typechecking result. - Issues *cel.Issues + Issues error // Err is the possible error that was encounter during type checking. Err error } @@ -168,26 +171,70 @@ func (c *TypeChecker) CreateContext(policy *v1beta1.ValidatingAdmissionPolicy) * } ctx.paramGVK = paramsGVK ctx.paramDeclType = paramsDeclType + ctx.variables = policy.Spec.Variables return ctx } +func (c *TypeChecker) compiler(ctx *TypeCheckingContext, typeOverwrite typeOverwrite) (*plugincel.CompositedCompiler, error) { + envSet, err := buildEnvSet( + /* hasParams */ ctx.paramDeclType != nil, + /* hasAuthorizer */ true, + typeOverwrite) + if err != nil { + return nil, err + } + env, err := plugincel.NewCompositionEnv(plugincel.VariablesTypeName, envSet) + if err != nil { + return nil, err + } + compiler := &plugincel.CompositedCompiler{ + Compiler: &typeCheckingCompiler{typeOverwrite: typeOverwrite, compositionEnv: env}, + CompositionEnv: env, + } + return compiler, nil +} + // CheckExpression type checks a single expression, given the context func (c *TypeChecker) CheckExpression(ctx *TypeCheckingContext, expression string) TypeCheckingResults { var results TypeCheckingResults for i, gvk := range ctx.gvks { declType := ctx.declTypes[i] - // TODO(jiahuif) hasAuthorizer always true for now, will change after expending type checking to all fields. - issues, err := c.checkExpression(expression, ctx.paramDeclType != nil, true, typeOverwrite{ + compiler, err := c.compiler(ctx, typeOverwrite{ object: declType, params: ctx.paramDeclType, }) - if issues != nil || err != nil { - results = append(results, &TypeCheckingResult{Issues: issues, Err: err, GVK: gvk}) + if err != nil { + utilruntime.HandleError(err) + continue + } + options := plugincel.OptionalVariableDeclarations{ + HasParams: ctx.paramDeclType != nil, + HasAuthorizer: true, + } + compiler.CompileAndStoreVariables(convertv1beta1Variables(ctx.variables), options, environment.StoredExpressions) + result := compiler.CompileCELExpression(celExpression(expression), options, environment.StoredExpressions) + if err := result.Error; err != nil { + typeCheckingResult := &TypeCheckingResult{GVK: gvk} + if err.Type == apiservercel.ErrorTypeInvalid { + typeCheckingResult.Issues = err + } else { + typeCheckingResult.Err = err + } + results = append(results, typeCheckingResult) } } return results } +type celExpression string + +func (c celExpression) GetExpression() string { + return string(c) +} + +func (c celExpression) ReturnTypes() []*cel.Type { + return []*cel.Type{cel.AnyType} +} func generateUniqueTypeName(kind string) string { return fmt.Sprintf("%s%d", kind, time.Now().Nanosecond()) } @@ -214,23 +261,6 @@ func (c *TypeChecker) paramsGVK(policy *v1beta1.ValidatingAdmissionPolicy) schem return gv.WithKind(policy.Spec.ParamKind.Kind) } -func (c *TypeChecker) checkExpression(expression string, hasParams, hasAuthorizer bool, types typeOverwrite) (*cel.Issues, error) { - env, err := buildEnv(hasParams, hasAuthorizer, types) - if err != nil { - return nil, err - } - - // We cannot reuse an AST that is parsed by another env, so reparse it here. - // Compile = Parse + Check, we especially want the results of Check. - // - // Paradoxically, we discard the type-checked result and let the admission - // controller use the dynamic typed program. - // This is a compromise that is defined in the KEP. We can revisit this - // decision and expect a change with limited size. - _, issues := env.Compile(expression) - return issues, nil -} - // typesToCheck extracts a list of GVKs that needs type checking from the policy // the result is sorted in the order of Group, Version, and Kind func (c *TypeChecker) typesToCheck(p *v1beta1.ValidatingAdmissionPolicy) []schema.GroupVersionKind { @@ -360,7 +390,7 @@ func (c *TypeChecker) tryRefreshRESTMapper() { } } -func buildEnv(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*cel.Env, error) { +func buildEnvSet(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*environment.EnvSet, error) { baseEnv := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()) requestType := plugincel.BuildRequestType() namespaceType := plugincel.BuildNamespaceType() @@ -392,7 +422,7 @@ func buildEnv(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*cel.Env varOpts = append(varOpts, cel.Variable("authorizer", library.AuthorizerType)) } - env, err := baseEnv.Extend( + return baseEnv.Extend( environment.VersionedOptions{ // Feature epoch was actually 1.26, but we artificially set it to 1.0 because these // options should always be present. @@ -401,10 +431,6 @@ func buildEnv(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*cel.Env DeclTypes: declTypes, }, ) - if err != nil { - return nil, err - } - return env.Env(environment.StoredExpressions) } // createVariableOpts creates a slice of EnvOption @@ -421,3 +447,40 @@ func createVariableOpts(declType *apiservercel.DeclType, variables ...string) [] } return opts } + +type typeCheckingCompiler struct { + compositionEnv *plugincel.CompositionEnv + typeOverwrite typeOverwrite +} + +// CompileCELExpression compiles the given expression. +// The implementation is the same as that of staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go +// except that: +// - object, oldObject, and params are typed instead of Dyn +// - compiler does not enforce the output type +// - the compiler does not initialize the program +func (c *typeCheckingCompiler) CompileCELExpression(expressionAccessor plugincel.ExpressionAccessor, options plugincel.OptionalVariableDeclarations, mode environment.Type) plugincel.CompilationResult { + resultError := func(errorString string, errType apiservercel.ErrorType) plugincel.CompilationResult { + return plugincel.CompilationResult{ + Error: &apiservercel.Error{ + Type: errType, + Detail: errorString, + }, + ExpressionAccessor: expressionAccessor, + } + } + env, err := c.compositionEnv.Env(mode) + if err != nil { + return resultError(fmt.Sprintf("fail to build env: %v", err), apiservercel.ErrorTypeInternal) + } + ast, issues := env.Compile(expressionAccessor.GetExpression()) + if issues != nil { + return resultError(issues.String(), apiservercel.ErrorTypeInvalid) + } + // type checker does not require the program, however the type must still be set. + return plugincel.CompilationResult{ + OutputType: ast.OutputType(), + } +} + +var _ plugincel.Compiler = (*typeCheckingCompiler)(nil) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking_test.go index e4b9a5d153d..67a0e98aec3 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/typechecking_test.go @@ -413,6 +413,124 @@ func TestTypeCheck(t *testing.T) { toContain("found no matching overload for 'allowed' applied to 'kubernetes.authorization.Authorizer"), }, }, + { + name: "variables valid", + policy: &v1beta1.ValidatingAdmissionPolicy{Spec: v1beta1.ValidatingAdmissionPolicySpec{ + Variables: []v1beta1.Variable{ + { + Name: "works", + Expression: "true", + }, + }, + Validations: []v1beta1.Validation{ + { + Expression: "variables.works", + }, + }, + MatchConstraints: deploymentPolicy.Spec.MatchConstraints, + }, + }, + schemaToReturn: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "foo": *spec.Int64Property(), + }, + }, + }, + assertions: []assertionFunc{toHaveLengthOf(0)}, + }, + { + name: "variables missing field", + policy: &v1beta1.ValidatingAdmissionPolicy{Spec: v1beta1.ValidatingAdmissionPolicySpec{ + Variables: []v1beta1.Variable{ + { + Name: "works", + Expression: "true", + }, + }, + Validations: []v1beta1.Validation{ + { + Expression: "variables.nonExisting", + }, + }, + MatchConstraints: deploymentPolicy.Spec.MatchConstraints, + }, + }, + schemaToReturn: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "foo": *spec.Int64Property(), + }, + }, + }, + assertions: []assertionFunc{ + toHaveLengthOf(1), + toHaveFieldRef("spec.validations[0].expression"), + toContain("undefined field 'nonExisting'"), + }, + }, + { + name: "variables field wrong type", + policy: &v1beta1.ValidatingAdmissionPolicy{Spec: v1beta1.ValidatingAdmissionPolicySpec{ + Variables: []v1beta1.Variable{ + { + Name: "name", + Expression: "'something'", + }, + }, + Validations: []v1beta1.Validation{ + { + Expression: "variables.name == object.foo", // foo is int64 + }, + }, + MatchConstraints: deploymentPolicy.Spec.MatchConstraints, + }, + }, + schemaToReturn: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "foo": *spec.Int64Property(), + }, + }, + }, + assertions: []assertionFunc{ + toHaveLengthOf(1), + toHaveFieldRef("spec.validations[0].expression"), + toContain("found no matching overload for '_==_' applied to '(string, int)"), + }, + }, + { + name: "error in variables, not reported during type checking.", + policy: &v1beta1.ValidatingAdmissionPolicy{Spec: v1beta1.ValidatingAdmissionPolicySpec{ + Variables: []v1beta1.Variable{ + { + Name: "name", + Expression: "object.foo == 'str'", + }, + }, + Validations: []v1beta1.Validation{ + { + Expression: "variables.name == object.foo", // foo is int64 + }, + }, + MatchConstraints: deploymentPolicy.Spec.MatchConstraints, + }, + }, + schemaToReturn: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "foo": *spec.Int64Property(), + }, + }, + }, + assertions: []assertionFunc{ + toHaveLengthOf(0), + }, + }, } { t.Run(tc.name, func(t *testing.T) { typeChecker := buildTypeChecker(tc.schemaToReturn)