Skip to content

Commit

Permalink
Merge pull request #823 from viccuad/matchconds-verify-rebased
Browse files Browse the repository at this point in the history
feat: verify MatchConditons, return aggregate errors on validation
  • Loading branch information
viccuad committed Jul 23, 2024
2 parents 588129f + 63c84ca commit e8dad5c
Show file tree
Hide file tree
Showing 11 changed files with 380 additions and 191 deletions.
6 changes: 6 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -373,3 +373,9 @@ issues:
- path: 'api/policies/v1/(.+)_webhook.go'
linters:
- dupl
- path: "api/policies/v1/policy_validation_matchconditions.go"
# this file is imported from k8s.io/kubernetes, ignore lint errors
linters:
- errorlint
- gochecknoglobals
- mnd
14 changes: 13 additions & 1 deletion api/policies/v1/admissionpolicy_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -65,7 +66,18 @@ var _ webhook.Validator = &AdmissionPolicy{}
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (r *AdmissionPolicy) ValidateCreate() (admission.Warnings, error) {
admissionpolicylog.Info("validate create", "name", r.Name)
return nil, validateRulesField(r)
errList := field.ErrorList{}

if errs := validateRulesField(r); len(errs) != 0 {
errList = append(errList, errs...)
}
if errs := validateMatchConditionsField(r); len(errs) != 0 {
errList = append(errList, errs...)
}
if len(errList) != 0 {
return nil, prepareInvalidAPIError(r, errList)
}
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
Expand Down
2 changes: 1 addition & 1 deletion api/policies/v1/admissionpolicy_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestAdmissionPolicyValidateUpdate(t *testing.T) {
}

func TestAdmissionPolicyValidateUpdateWithInvalidOldPolicy(t *testing.T) {
oldPolicy := clusterAdmissionPolicyFactory(nil, "", "protect")
oldPolicy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
newPolicy := admissionPolicyFactory()
warnings, err := newPolicy.ValidateUpdate(oldPolicy)
require.Empty(t, warnings)
Expand Down
13 changes: 12 additions & 1 deletion api/policies/v1/clusteradmissionpolicy_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"

"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -67,8 +68,18 @@ var _ webhook.Validator = &ClusterAdmissionPolicy{}
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (r *ClusterAdmissionPolicy) ValidateCreate() (admission.Warnings, error) {
clusteradmissionpolicylog.Info("validate create", "name", r.Name)
errList := field.ErrorList{}

return nil, validateRulesField(r)
if errs := validateRulesField(r); len(errs) != 0 {
errList = append(errList, errs...)
}
if errs := validateMatchConditionsField(r); len(errs) != 0 {
errList = append(errList, errs...)
}
if len(errList) != 0 {
return nil, prepareInvalidAPIError(r, errList)
}
return nil, nil
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
Expand Down
10 changes: 5 additions & 5 deletions api/policies/v1/clusteradmissionpolicy_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,31 @@ import (
)

func TestClusterAdmissionPolicyDefault(t *testing.T) {
policy := clusterAdmissionPolicyFactory(nil, "", "protect")
policy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
policy.Default()

require.Equal(t, constants.DefaultPolicyServer, policy.GetPolicyServer())
require.Contains(t, policy.GetFinalizers(), constants.KubewardenFinalizer)
}

func TestClusterAdmissionPolicyValidateCreate(t *testing.T) {
policy := clusterAdmissionPolicyFactory(nil, "", "protect")
policy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
warnings, err := policy.ValidateCreate()
require.NoError(t, err)
require.Empty(t, warnings)
}

func TestClusterAdmissionPolicyValidateUpdate(t *testing.T) {
oldPolicy := clusterAdmissionPolicyFactory(nil, "", "protect")
newPolicy := clusterAdmissionPolicyFactory(nil, "", "protect")
oldPolicy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
newPolicy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
warnings, err := newPolicy.ValidateUpdate(oldPolicy)
require.NoError(t, err)
require.Empty(t, warnings)
}

func TestClusterAdmissionPolicyValidateUpdateWithInvalidOldPolicy(t *testing.T) {
oldPolicy := admissionPolicyFactory()
newPolicy := clusterAdmissionPolicyFactory(nil, "", "protect")
newPolicy := clusterAdmissionPolicyFactory(nil, nil, "", "protect")
warnings, err := newPolicy.ValidateUpdate(oldPolicy)
require.Empty(t, warnings)
require.ErrorContains(t, err, "object is not of type ClusterAdmissionPolicy")
Expand Down
7 changes: 4 additions & 3 deletions api/policies/v1/policy_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func admissionPolicyFactory() *AdmissionPolicy {
}
}

func clusterAdmissionPolicyFactory(customRules []admissionregistrationv1.RuleWithOperations, policyServer string, policyMode PolicyMode) *ClusterAdmissionPolicy {
func clusterAdmissionPolicyFactory(customRules []admissionregistrationv1.RuleWithOperations, matchConds []admissionregistrationv1.MatchCondition, policyServer string, policyMode PolicyMode) *ClusterAdmissionPolicy {
return &ClusterAdmissionPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "testing-policy",
Expand All @@ -51,8 +51,9 @@ func clusterAdmissionPolicyFactory(customRules []admissionregistrationv1.RuleWit
Settings: runtime.RawExtension{
Raw: []byte("{}"),
},
Rules: getRules(customRules),
Mode: policyMode,
Rules: getRules(customRules),
Mode: policyMode,
MatchConditions: matchConds,
},
},
}
Expand Down
54 changes: 40 additions & 14 deletions api/policies/v1/policy_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,20 @@ package v1
import (
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"

apierrors "k8s.io/apimachinery/pkg/api/errors"
)

// Validates the spec.Rules field for non-empty, webhook-valid rules.
func validateRulesField(policy Policy) error {
func validateRulesField(policy Policy) field.ErrorList {
errs := field.ErrorList{}
rulesField := field.NewPath("spec", "rules")

if len(policy.GetRules()) == 0 {
errs = append(errs, field.Required(rulesField, "a value must be specified"))

return prepareInvalidAPIError(policy, errs)
return errs
}

for _, rule := range policy.GetRules() {
Expand All @@ -59,7 +58,7 @@ func validateRulesField(policy Policy) error {
}

if len(errs) != 0 {
return prepareInvalidAPIError(policy, errs)
return errs
}

return nil
Expand Down Expand Up @@ -91,6 +90,30 @@ func checkRulesArrayForEmptyString(rulesArray []string, fieldName string, parent
return nil
}

func validateMatchConditionsField(policy Policy) field.ErrorList {
// taken from the configuration for validating MutatingWebhookConfiguration:
// https://github.com/kubernetes/kubernetes/blob/c052f64689ee26aace4689f2433c5c7dcf1931ad/pkg/apis/admissionregistration/validation/validation.go#L257
opts := validationOptions{
ignoreMatchConditions: false,
allowParamsInMatchConditions: false,
requireNoSideEffects: true,
requireRecognizedAdmissionReviewVersion: true,
requireUniqueWebhookNames: true,
allowInvalidLabelValueInSelector: false,
// strictCostEnforcement enables cost enforcement for CEL.
// This is enabled with the StrictCostEnforcementForWebhooks feature gate
// (alpha on v1.30). Don't check it for now. Nevertheless, will get
// checked by the k8s API on WebhookConfiguration creation if the feature
// gate is enabled.
strictCostEnforcement: false,
}

if errs := validateMatchConditions(policy.GetMatchConditions(), opts, field.NewPath("spec").Child("matchConditions")); len(errs) != 0 {
return errs
}
return nil
}

// prepareInvalidAPIError is a shorthand for generating an invalid apierrors.StatusError with data from a policy.
func prepareInvalidAPIError(policy Policy, errorList field.ErrorList) *apierrors.StatusError {
return apierrors.NewInvalid(
Expand All @@ -101,31 +124,34 @@ func prepareInvalidAPIError(policy Policy, errorList field.ErrorList) *apierrors
}

func validatePolicyUpdate(oldPolicy, newPolicy Policy) error {
if err := validateRulesField(newPolicy); err != nil {
return err
errList := field.ErrorList{}

if errs := validateRulesField(newPolicy); len(errs) != 0 {
errList = append(errList, errs...)
}

if errs := validateMatchConditionsField(newPolicy); len(errs) != 0 {
errList = append(errList, errs...)
}

if newPolicy.GetPolicyServer() != oldPolicy.GetPolicyServer() {
var errs field.ErrorList
p := field.NewPath("spec")
pp := p.Child("policyServer")
errs = append(errs, field.Forbidden(pp, "the field is immutable"))

return apierrors.NewInvalid(
schema.GroupKind{Group: GroupVersion.Group, Kind: "ClusterAdmissionPolicy"},
newPolicy.GetName(), errs)
errList = append(errList, errs...)
}

if newPolicy.GetPolicyMode() == "monitor" && oldPolicy.GetPolicyMode() == "protect" {
var errs field.ErrorList
p := field.NewPath("spec")
pp := p.Child("mode")
errs = append(errs, field.Forbidden(pp, "field cannot transition from protect to monitor. Recreate instead."))

return apierrors.NewInvalid(
schema.GroupKind{Group: GroupVersion.Group, Kind: "ClusterAdmissionPolicy"},
newPolicy.GetName(), errs)
errList = append(errList, errs...)
}

if len(errList) != 0 {
return prepareInvalidAPIError(newPolicy, errList)
}
return nil
}
140 changes: 140 additions & 0 deletions api/policies/v1/policy_validation_matchconditions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package v1

/*
The following code is taken from the Kubernetes source code
at pkg/apis/admissionregistration/validation/validation.go
*/

/*
Copyright 2017 The Kubernetes 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.
*/

import (
"fmt"
"strings"

admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
plugincel "k8s.io/apiserver/pkg/admission/plugin/cel"
"k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions"
"k8s.io/apiserver/pkg/cel"
"k8s.io/apiserver/pkg/cel/environment"
)

type validationOptions struct {
ignoreMatchConditions bool
allowParamsInMatchConditions bool
requireNoSideEffects bool
requireRecognizedAdmissionReviewVersion bool
requireUniqueWebhookNames bool
allowInvalidLabelValueInSelector bool
preexistingExpressions preexistingExpressions
strictCostEnforcement bool
}

type preexistingExpressions struct {
matchConditionExpressions sets.Set[string]
}

func validateMatchConditions(m []admissionregistrationv1.MatchCondition, opts validationOptions, fldPath *field.Path) field.ErrorList {
var allErrors field.ErrorList
conditionNames := sets.NewString()
if len(m) > 64 {
allErrors = append(allErrors, field.TooMany(fldPath, len(m), 64))
}
for i, matchCondition := range m {
allErrors = append(allErrors, validateMatchCondition(&matchCondition, opts, fldPath.Index(i))...)
if len(matchCondition.Name) > 0 {
if conditionNames.Has(matchCondition.Name) {
allErrors = append(allErrors, field.Duplicate(fldPath.Index(i).Child("name"), matchCondition.Name))
} else {
conditionNames.Insert(matchCondition.Name)
}
}
}
return allErrors
}

func validateMatchCondition(v *admissionregistrationv1.MatchCondition, opts validationOptions, fldPath *field.Path) field.ErrorList {
var allErrors field.ErrorList
trimmedExpression := strings.TrimSpace(v.Expression)
if len(trimmedExpression) == 0 {
allErrors = append(allErrors, field.Required(fldPath.Child("expression"), ""))
} else {
allErrors = append(allErrors, validateMatchConditionsExpression(trimmedExpression, opts, fldPath.Child("expression"))...)
}
if len(v.Name) == 0 {
allErrors = append(allErrors, field.Required(fldPath.Child("name"), ""))
} else {
for _, msg := range validation.IsQualifiedName(v.Name) {
allErrors = append(allErrors, field.Invalid(fldPath, v.Name, msg))
}
}
return allErrors
}

func validateCELCondition(compiler plugincel.Compiler, expression plugincel.ExpressionAccessor, variables plugincel.OptionalVariableDeclarations, envType environment.Type, fldPath *field.Path) field.ErrorList {
var allErrors field.ErrorList
result := compiler.CompileCELExpression(expression, variables, envType)
if result.Error != nil {
allErrors = append(allErrors, convertCELErrorToValidationError(fldPath, expression, result.Error))
}
return allErrors
}

func convertCELErrorToValidationError(fldPath *field.Path, expression plugincel.ExpressionAccessor, err error) *field.Error {
if celErr, ok := err.(*cel.Error); ok {
switch celErr.Type {
case cel.ErrorTypeRequired:
return field.Required(fldPath, celErr.Detail)
case cel.ErrorTypeInvalid:
return field.Invalid(fldPath, expression.GetExpression(), celErr.Detail)
case cel.ErrorTypeInternal:
return field.InternalError(fldPath, celErr)
}
}
return field.InternalError(fldPath, fmt.Errorf("unsupported error type: %w", err))
}

func validateMatchConditionsExpression(expression string, opts validationOptions, fldPath *field.Path) field.ErrorList {
envType := environment.NewExpressions
if opts.preexistingExpressions.matchConditionExpressions.Has(expression) {
envType = environment.StoredExpressions
}
var compiler plugincel.Compiler
if opts.strictCostEnforcement {
compiler = strictStatelessCELCompiler
} else {
compiler = nonStrictStatelessCELCompiler
}
return validateCELCondition(compiler, &matchconditions.MatchCondition{
Expression: expression,
}, plugincel.OptionalVariableDeclarations{
HasParams: opts.allowParamsInMatchConditions,
HasAuthorizer: true,
StrictCost: opts.strictCostEnforcement,
}, envType, fldPath)
}

// statelessCELCompiler does not support variable composition (and thus is stateless). It should be used when
// variable composition is not allowed, for example, when validating MatchConditions.
// strictStatelessCELCompiler is a cel Compiler that enforces strict cost enforcement.
// nonStrictStatelessCELCompiler is a cel Compiler that does not enforce strict cost enforcement.
var (
strictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true))
nonStrictStatelessCELCompiler = plugincel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), false))
)
Loading

0 comments on commit e8dad5c

Please sign in to comment.