diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicy/authz_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicy/authz_test.go index dc4a7e5d017..8b2e56a7baa 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicy/authz_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicy/authz_test.go @@ -20,10 +20,12 @@ import ( "context" "testing" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/kubernetes/pkg/apis/admissionregistration" "k8s.io/kubernetes/pkg/registry/admissionregistration/resolver" ) @@ -31,6 +33,7 @@ func TestAuthorization(t *testing.T) { for _, tc := range []struct { name string userInfo user.Info + obj *admissionregistration.ValidatingAdmissionPolicy auth AuthFunc resourceResolver resolver.ResourceResolverFunc expectErr bool @@ -39,6 +42,7 @@ func TestAuthorization(t *testing.T) { name: "superuser", userInfo: &user.DefaultInfo{Groups: []string{user.SystemPrivilegedGroup}}, expectErr: false, // success despite always-denying authorizer + obj: validValidatingAdmissionPolicy(), auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { return authorizer.DecisionDeny, "", nil }, @@ -46,6 +50,7 @@ func TestAuthorization(t *testing.T) { { name: "authorized", userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + obj: validValidatingAdmissionPolicy(), auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { if a.GetResource() == "replicalimits" { return authorizer.DecisionAllow, "", nil @@ -64,6 +69,7 @@ func TestAuthorization(t *testing.T) { { name: "denied", userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + obj: validValidatingAdmissionPolicy(), auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { if a.GetResource() == "configmaps" { return authorizer.DecisionAllow, "", nil @@ -79,22 +85,36 @@ func TestAuthorization(t *testing.T) { }, expectErr: true, }, + { + name: "param not found", + userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + obj: validValidatingAdmissionPolicy(), + auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + if a.GetResource() == "replicalimits" { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "", nil + }, + resourceResolver: func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{}, &meta.NoKindMatchError{GroupKind: gvk.GroupKind(), SearchedVersions: []string{gvk.Version}} + }, + expectErr: true, + }, } { t.Run(tc.name, func(t *testing.T) { strategy := NewStrategy(tc.auth, tc.resourceResolver) t.Run("create", func(t *testing.T) { ctx := request.WithUser(context.Background(), tc.userInfo) - errs := strategy.Validate(ctx, validValidatingAdmissionPolicy()) + errs := strategy.Validate(ctx, tc.obj) if len(errs) > 0 != tc.expectErr { t.Errorf("expected error: %v but got error: %v", tc.expectErr, errs) } }) t.Run("update", func(t *testing.T) { ctx := request.WithUser(context.Background(), tc.userInfo) - obj := validValidatingAdmissionPolicy() - objWithUpdatedParamKind := obj.DeepCopy() + objWithUpdatedParamKind := tc.obj.DeepCopy() objWithUpdatedParamKind.Spec.ParamKind.APIVersion += "1" - errs := strategy.ValidateUpdate(ctx, obj, objWithUpdatedParamKind) + errs := strategy.ValidateUpdate(ctx, tc.obj, objWithUpdatedParamKind) if len(errs) > 0 != tc.expectErr { t.Errorf("expected error: %v but got error: %v", tc.expectErr, errs) } diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go index 29c35559873..98112f765bd 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" @@ -201,7 +202,7 @@ func newValidatingAdmissionPolicy(name string) *admissionregistration.Validating } func newInsecureStorage(t *testing.T) (*REST, *etcd3testing.EtcdTestServer) { - return newStorage(t, nil, nil) + return newStorage(t, nil, replicaLimitsResolver) } func newStorage(t *testing.T, authorizer authorizer.Authorizer, resourceResolver resolver.ResourceResolver) (*REST, *etcd3testing.EtcdTestServer) { @@ -225,3 +226,11 @@ func TestCategories(t *testing.T) { expected := []string{"api-extensions"} registrytest.AssertCategories(t, storage, expected) } + +var replicaLimitsResolver resolver.ResourceResolverFunc = func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "rules.example.com", + Version: "v1", + Resource: "replicalimits", + }, nil +} diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicy/strategy_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicy/strategy_test.go index cb993f72f71..03282596779 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicy/strategy_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicy/strategy_test.go @@ -20,12 +20,14 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/kubernetes/pkg/apis/admissionregistration" + "k8s.io/kubernetes/pkg/registry/admissionregistration/resolver" ) func TestValidatingAdmissionPolicyStrategy(t *testing.T) { - strategy := NewStrategy(nil, nil) + strategy := NewStrategy(nil, replicaLimitsResolver) ctx := genericapirequest.NewDefaultContext() if strategy.NamespaceScoped() { t.Error("ValidatingAdmissionPolicy strategy must be cluster scoped") @@ -49,6 +51,15 @@ func TestValidatingAdmissionPolicyStrategy(t *testing.T) { t.Errorf("Expected a validation error") } } + +var replicaLimitsResolver resolver.ResourceResolverFunc = func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "rules.example.com", + Version: "v1", + Resource: "replicalimits", + }, nil +} + func validValidatingAdmissionPolicy() *admissionregistration.ValidatingAdmissionPolicy { ignore := admissionregistration.Ignore return &admissionregistration.ValidatingAdmissionPolicy{ diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go index 77ac8b2bd47..68466c37f0e 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz.go @@ -55,7 +55,10 @@ func (v *validatingAdmissionPolicyBindingStrategy) authorizeUpdate(ctx context.C } func (v *validatingAdmissionPolicyBindingStrategy) authorize(ctx context.Context, binding *admissionregistration.ValidatingAdmissionPolicyBinding) error { - if v.authorizer == nil || v.resourceResolver == nil || binding.Spec.ParamRef == nil { + if v.resourceResolver == nil { + return fmt.Errorf(`unexpected internal error: resourceResolver is nil`) + } + if v.authorizer == nil || binding.Spec.ParamRef == nil { return nil } @@ -72,13 +75,21 @@ func (v *validatingAdmissionPolicyBindingStrategy) authorize(ctx context.Context // default to requiring permissions on all group/version/resources resource, apiGroup, apiVersion := "*", "*", "*" - if policy, err := v.policyGetter.GetValidatingAdmissionPolicy(ctx, binding.Spec.PolicyName); err == nil && policy.Spec.ParamKind != nil { + var policyErr, gvParseErr, gvrResolveErr error + + var policy *admissionregistration.ValidatingAdmissionPolicy + policy, policyErr = v.policyGetter.GetValidatingAdmissionPolicy(ctx, binding.Spec.PolicyName) + if policyErr == nil && policy.Spec.ParamKind != nil { paramKind := policy.Spec.ParamKind - if gv, err := schema.ParseGroupVersion(paramKind.APIVersion); err == nil { + var gv schema.GroupVersion + gv, gvParseErr = schema.ParseGroupVersion(paramKind.APIVersion) + if gvParseErr == nil { // we only need to authorize the parsed group/version apiGroup = gv.Group apiVersion = gv.Version - if gvr, err := v.resourceResolver.Resolve(gv.WithKind(paramKind.Kind)); err == nil { + var gvr schema.GroupVersionResource + gvr, gvrResolveErr = v.resourceResolver.Resolve(gv.WithKind(paramKind.Kind)) + if gvrResolveErr == nil { // we only need to authorize the resolved resource resource = gvr.Resource } @@ -107,9 +118,18 @@ func (v *validatingAdmissionPolicyBindingStrategy) authorize(ctx context.Context d, _, err := v.authorizer.Authorize(ctx, attrs) if err != nil { - return err + return fmt.Errorf(`failed to authorize request: %w`, err) } if d != authorizer.DecisionAllow { + if policyErr != nil { + return fmt.Errorf(`unable to get policy %s to determine minimum required permissions and user %v does not have "%v" permission for all groups, versions and resources`, binding.Spec.PolicyName, user, verb) + } + if gvParseErr != nil { + return fmt.Errorf(`unable to parse paramKind %v to determine minimum required permissions and user %v does not have "%v" permission for all groups, versions and resources`, policy.Spec.ParamKind, user, verb) + } + if gvrResolveErr != nil { + return fmt.Errorf(`unable to resolve paramKind %v to determine minimum required permissions and user %v does not have "%v" permission for all groups, versions and resources`, policy.Spec.ParamKind, user, verb) + } return fmt.Errorf(`user %v does not have "%v" permission on the object referenced by paramRef`, user, verb) } diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz_test.go index c72e9f92d2a..554374f0e40 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/authz_test.go @@ -18,8 +18,12 @@ package validatingadmissionpolicybinding import ( "context" + "errors" + "fmt" + "strings" "testing" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/authentication/user" @@ -31,17 +35,16 @@ import ( func TestAuthorization(t *testing.T) { for _, tc := range []struct { - name string - userInfo user.Info - auth AuthFunc - policyGetter PolicyGetterFunc - resourceResolver resolver.ResourceResolverFunc - expectErr bool + name string + userInfo user.Info + auth AuthFunc + policyGetter PolicyGetterFunc + resourceResolver resolver.ResourceResolverFunc + expectErrContains string }{ { - name: "superuser", - userInfo: &user.DefaultInfo{Groups: []string{user.SystemPrivilegedGroup}}, - expectErr: false, // success despite always-denying authorizer + name: "superuser", // success despite always-denying authorizer + userInfo: &user.DefaultInfo{Groups: []string{user.SystemPrivilegedGroup}}, auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { return authorizer.DecisionDeny, "", nil }, @@ -70,7 +73,6 @@ func TestAuthorization(t *testing.T) { Resource: "configmaps", }, nil }, - expectErr: false, }, { name: "denied", @@ -96,7 +98,76 @@ func TestAuthorization(t *testing.T) { Resource: "params", }, nil }, - expectErr: true, + expectErrContains: "permission on the object referenced by paramRef", + }, + { + name: "unable to parse paramRef", + userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + if a.GetResource() == "configmaps" { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "", nil + }, + policyGetter: func(ctx context.Context, name string) (*admissionregistration.ValidatingAdmissionPolicy, error) { + return &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "replicalimit-policy.example.com"}, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + ParamKind: &admissionregistration.ParamKind{Kind: "ConfigMap", APIVersion: "invalid"}, + }, + }, nil + }, + resourceResolver: func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "configmaps", + }, nil + }, + expectErrContains: "unable to parse paramKind &{foo.example.com/v1 Params} to determine minimum required permissions", + }, + { + name: "unable to resolve param", + userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + if a.GetResource() == "configmaps" { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "", nil + }, + policyGetter: func(ctx context.Context, name string) (*admissionregistration.ValidatingAdmissionPolicy, error) { + return &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "replicalimit-policy.example.com"}, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + ParamKind: &admissionregistration.ParamKind{Kind: "Params", APIVersion: "foo.example.com/v1"}, + }, + }, nil + }, + resourceResolver: func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{}, &meta.NoKindMatchError{GroupKind: gvk.GroupKind(), SearchedVersions: []string{gvk.Version}} + }, + expectErrContains: "unable to resolve paramKind &{foo.example.com/v1 Params} to determine minimum required permissions", + }, + { + name: "unable to get policy", + userInfo: &user.DefaultInfo{Groups: []string{user.AllAuthenticated}}, + auth: func(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + if a.GetResource() == "configmaps" { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "", nil + }, + policyGetter: func(ctx context.Context, name string) (*admissionregistration.ValidatingAdmissionPolicy, error) { + return nil, fmt.Errorf("no such policy") + }, + resourceResolver: func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "configmaps", + }, nil + }, + expectErrContains: "unable to get policy replicalimit-policy.example.com to determine minimum required permissions", }, } { t.Run(tc.name, func(t *testing.T) { @@ -105,8 +176,8 @@ func TestAuthorization(t *testing.T) { ctx := request.WithUser(context.Background(), tc.userInfo) for _, obj := range validPolicyBindings() { errs := strategy.Validate(ctx, obj) - if len(errs) > 0 != tc.expectErr { - t.Errorf("expected error: %v but got error: %v", tc.expectErr, errs) + if len(errs) > 0 && !strings.Contains(errors.Join(errs.ToAggregate().Errors()...).Error(), tc.expectErrContains) { + t.Errorf("expected error to contain: %v but got error: %v", tc.expectErrContains, errs) } } }) @@ -140,8 +211,8 @@ func TestAuthorization(t *testing.T) { } } errs := strategy.ValidateUpdate(ctx, obj, objWithChangedParamRef) - if len(errs) > 0 != tc.expectErr { - t.Errorf("expected error: %v but got error: %v", tc.expectErr, errs) + if len(errs) > 0 && !strings.Contains(errors.Join(errs.ToAggregate().Errors()...).Error(), tc.expectErrContains) { + t.Errorf("expected error to contain: %v but got error: %v", tc.expectErrContains, errs) } } }) diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go index f177f57ea67..d679b76d12c 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" @@ -230,7 +231,7 @@ func newPolicyBinding(name string) *admissionregistration.ValidatingAdmissionPol } func newInsecureStorage(t *testing.T) (*REST, *etcd3testing.EtcdTestServer) { - return newStorage(t, nil, nil, nil) + return newStorage(t, nil, nil, replicaLimitsResolver) } func newStorage(t *testing.T, authorizer authorizer.Authorizer, policyGetter PolicyGetter, resourceResolver resolver.ResourceResolver) (*REST, *etcd3testing.EtcdTestServer) { @@ -254,3 +255,11 @@ func TestCategories(t *testing.T) { expected := []string{"api-extensions"} registrytest.AssertCategories(t, storage, expected) } + +var replicaLimitsResolver resolver.ResourceResolverFunc = func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "rules.example.com", + Version: "v1", + Resource: "replicalimits", + }, nil +} diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go index 55b310db31a..cf26c09075e 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go @@ -20,13 +20,15 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/kubernetes/pkg/registry/admissionregistration/resolver" "k8s.io/kubernetes/pkg/apis/admissionregistration" ) func TestPolicyBindingStrategy(t *testing.T) { - strategy := NewStrategy(nil, nil, nil) + strategy := NewStrategy(nil, nil, replicaLimitsResolver) ctx := genericapirequest.NewDefaultContext() if strategy.NamespaceScoped() { t.Error("PolicyBinding strategy must be cluster scoped") @@ -52,6 +54,14 @@ func TestPolicyBindingStrategy(t *testing.T) { } } +var replicaLimitsResolver resolver.ResourceResolverFunc = func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { + return schema.GroupVersionResource{ + Group: "rules.example.com", + Version: "v1", + Resource: "replicalimits", + }, nil +} + func validPolicyBindings() []*admissionregistration.ValidatingAdmissionPolicyBinding { denyAction := admissionregistration.DenyAction return []*admissionregistration.ValidatingAdmissionPolicyBinding{ diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer.go deleted file mode 100644 index ac13dbeee37..00000000000 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer.go +++ /dev/null @@ -1,152 +0,0 @@ -/* -Copyright 2023 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. -*/ - -package validating - -import ( - "context" - "encoding/json" - "sort" - "strings" - - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" -) - -type authzResult struct { - authorized authorizer.Decision - reason string - err error -} - -type cachingAuthorizer struct { - authorizer authorizer.Authorizer - decisions map[string]authzResult -} - -func newCachingAuthorizer(in authorizer.Authorizer) authorizer.Authorizer { - return &cachingAuthorizer{ - authorizer: in, - decisions: make(map[string]authzResult), - } -} - -// The attribute accessors known to cache key construction. If this fails to compile, the cache -// implementation may need to be updated. -var _ authorizer.Attributes = (interface { - GetUser() user.Info - GetVerb() string - IsReadOnly() bool - GetNamespace() string - GetResource() string - GetSubresource() string - GetName() string - GetAPIGroup() string - GetAPIVersion() string - IsResourceRequest() bool - GetPath() string - GetFieldSelector() (fields.Requirements, error) - GetLabelSelector() (labels.Requirements, error) -})(nil) - -// The user info accessors known to cache key construction. If this fails to compile, the cache -// implementation may need to be updated. -var _ user.Info = (interface { - GetName() string - GetUID() string - GetGroups() []string - GetExtra() map[string][]string -})(nil) - -// Authorize returns an authorization decision by delegating to another Authorizer. If an equivalent -// check has already been performed, a cached result is returned. Not safe for concurrent use. -func (ca *cachingAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { - type SerializableAttributes struct { - authorizer.AttributesRecord - LabelSelector string - } - - serializableAttributes := SerializableAttributes{ - AttributesRecord: authorizer.AttributesRecord{ - Verb: a.GetVerb(), - Namespace: a.GetNamespace(), - APIGroup: a.GetAPIGroup(), - APIVersion: a.GetAPIVersion(), - Resource: a.GetResource(), - Subresource: a.GetSubresource(), - Name: a.GetName(), - ResourceRequest: a.IsResourceRequest(), - Path: a.GetPath(), - }, - } - // in the error case, we won't honor this field selector, so the cache doesn't need it. - if fieldSelector, err := a.GetFieldSelector(); len(fieldSelector) > 0 { - serializableAttributes.FieldSelectorRequirements, serializableAttributes.FieldSelectorParsingErr = fieldSelector, err - } - if labelSelector, _ := a.GetLabelSelector(); len(labelSelector) > 0 { - // the labels requirements have private elements so those don't help us serialize to a unique key - serializableAttributes.LabelSelector = labelSelector.String() - } - - if u := a.GetUser(); u != nil { - di := &user.DefaultInfo{ - Name: u.GetName(), - UID: u.GetUID(), - } - - // Differently-ordered groups or extras could cause otherwise-equivalent checks to - // have distinct cache keys. - if groups := u.GetGroups(); len(groups) > 0 { - di.Groups = make([]string, len(groups)) - copy(di.Groups, groups) - sort.Strings(di.Groups) - } - - if extra := u.GetExtra(); len(extra) > 0 { - di.Extra = make(map[string][]string, len(extra)) - for k, vs := range extra { - vdupe := make([]string, len(vs)) - copy(vdupe, vs) - sort.Strings(vdupe) - di.Extra[k] = vdupe - } - } - - serializableAttributes.User = di - } - - var b strings.Builder - if err := json.NewEncoder(&b).Encode(serializableAttributes); err != nil { - return authorizer.DecisionNoOpinion, "", err - } - key := b.String() - - if cached, ok := ca.decisions[key]; ok { - return cached.authorized, cached.reason, cached.err - } - - authorized, reason, err := ca.authorizer.Authorize(ctx, a) - - ca.decisions[key] = authzResult{ - authorized: authorized, - reason: reason, - err: err, - } - - return authorized, reason, err -} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer_test.go deleted file mode 100644 index 5831cc3febf..00000000000 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/caching_authorizer_test.go +++ /dev/null @@ -1,523 +0,0 @@ -/* -Copyright 2023 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. -*/ - -package validating - -import ( - "context" - "errors" - "fmt" - "testing" - - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" - genericfeatures "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" -) - -func mustParseLabelSelector(str string) labels.Requirements { - ret, err := labels.Parse(str) - if err != nil { - panic(err) - } - retRequirements, _ /*selectable*/ := ret.Requirements() - return retRequirements -} - -func TestCachingAuthorizer(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.AuthorizeWithSelectors, true) - - type result struct { - decision authorizer.Decision - reason string - error error - } - - type invocation struct { - attributes authorizer.Attributes - expected result - } - - for _, tc := range []struct { - name string - calls []invocation - backend []result - }{ - { - name: "hit", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{Name: "test name"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{Name: "test name"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "hit with differently-ordered groups", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Groups: []string{"a", "b", "c"}, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Groups: []string{"c", "b", "a"}, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "hit with differently-ordered extra", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Extra: map[string][]string{ - "k": {"a", "b", "c"}, - }, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Extra: map[string][]string{ - "k": {"c", "b", "a"}, - }, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "miss due to different name", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{Name: "alpha"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - }, - { - attributes: authorizer.AttributesRecord{Name: "beta"}, - expected: result{ - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - { - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - { - name: "miss due to different user", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{Name: "alpha"}, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{Name: "beta"}, - }, - expected: result{ - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - { - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - { - name: "honor good field selector", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorRequirements: fields.ParseSelectorOrDie("foo=bar").Requirements(), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - { - // now this should be cached - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorRequirements: fields.ParseSelectorOrDie("foo=bar").Requirements(), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - }, - { - name: "ignore malformed field selector first", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorParsingErr: errors.New("malformed"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // notice that this does not have the malformed field selector. - // it should use the cached result - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "ignore malformed field selector second", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // this should use the broader cached value because the selector will be ignored - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorParsingErr: errors.New("malformed"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - - { - name: "honor good label selector", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("foo=bar"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - { - // now this should be cached - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("foo=bar"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("diff=zero"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 3", - error: fmt.Errorf("test error 3"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 3", - error: fmt.Errorf("test error 3"), - }, - }, - }, - { - name: "ignore malformed label selector first", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorParsingErr: errors.New("malformed mess"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // notice that this does not have the malformed field selector. - // it should use the cached result - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "ignore malformed label selector second", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // this should use the broader cached value because the selector will be ignored - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorParsingErr: errors.New("malformed mess"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - var misses int - frontend := newCachingAuthorizer(func() authorizer.Authorizer { - return authorizer.AuthorizerFunc(func(_ context.Context, attributes authorizer.Attributes) (authorizer.Decision, string, error) { - if misses >= len(tc.backend) { - t.Fatalf("got more than expected %d backend invocations", len(tc.backend)) - } - result := tc.backend[misses] - misses++ - return result.decision, result.reason, result.error - }) - }()) - - for i, invocation := range tc.calls { - decision, reason, err := frontend.Authorize(context.TODO(), invocation.attributes) - if decision != invocation.expected.decision { - t.Errorf("(call %d of %d) expected decision %v, got %v", i+1, len(tc.calls), invocation.expected.decision, decision) - } - if reason != invocation.expected.reason { - t.Errorf("(call %d of %d) expected reason %q, got %q", i+1, len(tc.calls), invocation.expected.reason, reason) - } - if err.Error() != invocation.expected.error.Error() { - t.Errorf("(call %d of %d) expected error %q, got %q", i+1, len(tc.calls), invocation.expected.error.Error(), err.Error()) - } - } - - if len(tc.backend) > misses { - t.Errorf("expected %d backend invocations, got %d", len(tc.backend), misses) - } - }) - } -} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go index b11f2e8f4eb..02ef81141d0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator.go @@ -41,13 +41,13 @@ import ( // validator implements the Validator interface type validator struct { celMatcher matchconditions.Matcher - validationFilter cel.Filter - auditAnnotationFilter cel.Filter - messageFilter cel.Filter + validationFilter cel.ConditionEvaluator + auditAnnotationFilter cel.ConditionEvaluator + messageFilter cel.ConditionEvaluator failPolicy *v1.FailurePolicyType } -func NewValidator(validationFilter cel.Filter, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.Filter, failPolicy *v1.FailurePolicyType) Validator { +func NewValidator(validationFilter cel.ConditionEvaluator, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.ConditionEvaluator, failPolicy *v1.FailurePolicyType) Validator { return &validator{ celMatcher: celMatcher, validationFilter: validationFilter, diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go index 44d79e60f88..eb5704da5c7 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/validator_test.go @@ -41,7 +41,7 @@ import ( "k8s.io/apiserver/pkg/cel/environment" ) -var _ cel.Filter = &fakeCelFilter{} +var _ cel.ConditionEvaluator = &fakeCelFilter{} type fakeCelFilter struct { evaluations []cel.EvaluationResult @@ -932,8 +932,8 @@ func TestContextCanceled(t *testing.T) { fakeAttr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "default", "foo", schema.GroupVersionResource{}, "", admission.Create, nil, false, nil) fakeVersionedAttr, _ := admission.NewVersionedAttributes(fakeAttr, schema.GroupVersionKind{}, nil) - fc := cel.NewFilterCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) - f := fc.Compile([]cel.ExpressionAccessor{&ValidationCondition{Expression: "[1,2,3,4,5,6,7,8,9,10].map(x, [1,2,3,4,5,6,7,8,9,10].map(y, x*y)) == []"}}, cel.OptionalVariableDeclarations{HasParams: false, HasAuthorizer: false}, environment.StoredExpressions) + fc := cel.NewConditionCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) + f := fc.CompileCondition([]cel.ExpressionAccessor{&ValidationCondition{Expression: "[1,2,3,4,5,6,7,8,9,10].map(x, [1,2,3,4,5,6,7,8,9,10].map(y, x*y)) == []"}}, cel.OptionalVariableDeclarations{HasParams: false, HasAuthorizer: false}, environment.StoredExpressions) v := validator{ failPolicy: &fail, celMatcher: &fakeCELMatcher{matches: true},