From 0dc08eded95c2d620de70648dee07254f2e771b3 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Mon, 4 Nov 2024 10:50:53 -0500 Subject: [PATCH] Reorganize and expand unit test coverage Also apply reviewer feedback --- .../validation/validation.go | 6 +- .../storage/storage_test.go | 4 +- .../policy/generic/policy_dispatcher.go | 4 +- .../plugin/policy/mutating/compilation.go | 4 +- .../policy/mutating/compilation_test.go | 638 +---------------- .../plugin/policy/mutating/dispatcher.go | 8 +- .../plugin/policy/mutating/dispatcher_test.go | 675 ++++++++++++++++++ .../plugin/policy/mutating/interface.go | 60 -- .../plugin/policy/mutating/patch/interface.go | 2 + .../policy/mutating/patch/json_patch.go | 19 + .../policy/mutating/patch/json_patch_test.go | 485 +++++++++++++ .../plugin/policy/mutating/patch/smd.go | 23 + .../plugin/policy/mutating/patch/smd_test.go | 375 ++++++++++ .../mutating/patch/typeconverter_test.go | 100 +++ .../plugin/policy/mutating/plugin.go | 2 +- .../plugin/policy/mutating/plugin_test.go | 62 ++ .../policy/mutating/reinvocationcontext.go | 2 +- .../mutating/reinvocationcontext_test.go | 147 ++++ .../plugin/policy/validating/typechecking.go | 2 +- .../pkg/cel/mutation/dynamic/objects.go | 4 +- .../apiserver/pkg/cel/mutation/jsonpatch.go | 11 +- 21 files changed, 1940 insertions(+), 693 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher_test.go delete mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/interface.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd_test.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter_test.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext_test.go diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 0fbf8833252..4953104033a 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -33,7 +33,7 @@ import ( utilvalidation "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/policy/mutating" + "k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch" validatingadmissionpolicy "k8s.io/apiserver/pkg/admission/plugin/policy/validating" "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" "k8s.io/apiserver/pkg/cel" @@ -1493,7 +1493,7 @@ func validateApplyConfiguration(compiler plugincel.Compiler, applyConfig *admiss if opts.preexistingExpressions.applyConfigurationExpressions.Has(applyConfig.Expression) { envType = environment.StoredExpressions } - accessor := &mutating.ApplyConfigurationCondition{ + accessor := &patch.ApplyConfigurationCondition{ Expression: trimmedExpression, } opts := plugincel.OptionalVariableDeclarations{HasParams: paramKind != nil, HasAuthorizer: true, StrictCost: true, HasPatchTypes: true} @@ -1516,7 +1516,7 @@ func validateJSONPatch(compiler plugincel.Compiler, jsonPatch *admissionregistra if opts.preexistingExpressions.applyConfigurationExpressions.Has(jsonPatch.Expression) { envType = environment.StoredExpressions } - accessor := &mutating.JSONPatchCondition{ + accessor := &patch.JSONPatchCondition{ Expression: trimmedExpression, } opts := plugincel.OptionalVariableDeclarations{HasParams: paramKind != nil, HasAuthorizer: true, StrictCost: true, HasPatchTypes: true} diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go index 98112f765bd..0b0046339bd 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicy/storage/storage_test.go @@ -202,7 +202,7 @@ func newValidatingAdmissionPolicy(name string) *admissionregistration.Validating } func newInsecureStorage(t *testing.T) (*REST, *etcd3testing.EtcdTestServer) { - return newStorage(t, nil, replicaLimitsResolver) + return newStorage(t, nil, resolver.ResourceResolverFunc(replicaLimitsResolver)) } func newStorage(t *testing.T, authorizer authorizer.Authorizer, resourceResolver resolver.ResourceResolver) (*REST, *etcd3testing.EtcdTestServer) { @@ -227,7 +227,7 @@ func TestCategories(t *testing.T) { registrytest.AssertCategories(t, storage, expected) } -var replicaLimitsResolver resolver.ResourceResolverFunc = func(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { +func replicaLimitsResolver(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { return schema.GroupVersionResource{ Group: "rules.example.com", Version: "v1", diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go index c520d38203c..62214a3092e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go @@ -95,7 +95,7 @@ func NewPolicyDispatcher[P runtime.Object, B runtime.Object, E Evaluator]( } } -// Start implements generic.Dispatcher Start. It loops through all active hooks +// Dispatch implements generic.Dispatcher. It loops through all active hooks // (policy x binding pairs) and selects those which are active for the current // request. It then resolves all params and creates an Invocation for each // matching policy-binding-param tuple. The delegate is then called with the @@ -413,5 +413,5 @@ func (c PolicyError) Error() string { return fmt.Sprintf("policy '%s' with binding '%s' denied request: %s", c.Policy.GetName(), c.Binding.GetName(), c.Message.Error()) } - return fmt.Sprintf("policy '%s' denied request: %s", c.Policy.GetName(), c.Message.Error()) + return fmt.Sprintf("policy %q denied request: %s", c.Policy.GetName(), c.Message.Error()) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation.go index 54d5938dd62..710b8ef1ea4 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation.go @@ -64,13 +64,13 @@ func compilePolicy(policy *Policy) PolicyEvaluator { switch m.PatchType { case v1alpha1.PatchTypeJSONPatch: if m.JSONPatch != nil { - accessor := &JSONPatchCondition{Expression: m.JSONPatch.Expression} + accessor := &patch.JSONPatchCondition{Expression: m.JSONPatch.Expression} compileResult := compiler.CompileMutatingEvaluator(accessor, patchOptions, environment.StoredExpressions) patchers = append(patchers, patch.NewJSONPatcher(compileResult)) } case v1alpha1.PatchTypeApplyConfiguration: if m.ApplyConfiguration != nil { - accessor := &ApplyConfigurationCondition{Expression: m.ApplyConfiguration.Expression} + accessor := &patch.ApplyConfigurationCondition{Expression: m.ApplyConfiguration.Expression} compileResult := compiler.CompileMutatingEvaluator(accessor, patchOptions, environment.StoredExpressions) patchers = append(patchers, patch.NewApplyConfigurationPatcher(compileResult)) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation_test.go index c9de4a343eb..ef0859d3a6e 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/compilation_test.go @@ -21,16 +21,17 @@ import ( "github.com/google/go-cmp/cmp" "strings" "testing" + "time" "k8s.io/api/admissionregistration/v1alpha1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch" @@ -45,6 +46,7 @@ import ( // on the results. func TestCompilation(t *testing.T) { deploymentGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + deploymentGVK := schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} testCases := []struct { name string policy *Policy @@ -56,429 +58,6 @@ func TestCompilation(t *testing.T) { expectedErr string expectedResult runtime.Object }{ - { - name: "jsonPatch with false test operation", - policy: jsonPatches(policy("d1"), - v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "test", path: "/spec/replicas", value: 100}, - JSONPatch{op: "replace", path: "/spec/replicas", value: 3}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - }, - { - name: "jsonPatch with true test operation", - policy: jsonPatches(policy("d1"), - v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "test", path: "/spec/replicas", value: 1}, - JSONPatch{op: "replace", path: "/spec/replicas", value: 3}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, - }, - { - name: "jsonPatch remove to unset field", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "remove", path: "/spec/replicas"}, - ]`, - }), - - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch remove map entry by key", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "remove", path: "/metadata/labels/y"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1", "y": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch remove element in list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "remove", path: "/spec/template/spec/containers/1"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "c"}}, - }}}}, - }, - { - name: "jsonPatch copy map entry by key", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "copy", from: "/metadata/labels/x", path: "/metadata/labels/y"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1", "y": "1"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch copy first element to end of list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "copy", from: "/spec/template/spec/containers/0", path: "/spec/template/spec/containers/-"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}, {Name: "a"}}, - }}}}, - }, - { - name: "jsonPatch move map entry by key", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "move", from: "/metadata/labels/x", path: "/metadata/labels/y"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch move first element to end of list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "move", from: "/spec/template/spec/containers/0", path: "/spec/template/spec/containers/-"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "b"}, {Name: "c"}, {Name: "a"}}, - }}}}, - }, - { - name: "jsonPatch add map entry by key and value", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/metadata/labels/x", value: "2"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1", "x": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch add map value to field", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/metadata/labels", value: {"y": "2"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch add map to existing map", // performs a replacement - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/metadata/labels", value: {"y": "2"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch add to start of list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/spec/template/spec/containers/0", value: {"name": "x"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "x"}, {Name: "a"}}, - }}}}, - }, - { - name: "jsonPatch add to end of list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/spec/template/spec/containers/-", value: {"name": "x"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "x"}}, - }}}}, - }, - { - name: "jsonPatch replace key in map", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/metadata/labels/x", value: "2"}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1", "x": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch replace map value of unset field", // adds the field value - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/metadata/labels", value: {"y": "2"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch replace map value of set field", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/metadata/labels", value: {"y": "2"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, - }, - { - name: "jsonPatch replace first element in list", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/spec/template/spec/containers/0", value: {"name": "x"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "x"}}, - }}}}, - }, - { - name: "jsonPatch replace end of list with - not allowed", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/spec/template/spec/containers/-", value: {"name": "x"}}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedErr: "JSON Patch: replace operation does not apply: doc is missing key: /spec/template/spec/containers/-: missing value", - }, - { - name: "jsonPatch replace with variable", - policy: jsonPatches(variables(policy("d1"), v1alpha1.Variable{Name: "desired", Expression: "10"}), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "replace", path: "/spec/replicas", value: variables.desired + 1}, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](11)}}, - }, - { - name: "jsonPatch with CEL initializer", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/spec/template/spec/containers/-", value: Object.spec.template.spec.containers{ - name: "x", - ports: [Object.spec.template.spec.containers.ports{containerPort: 8080}], - } - }, - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}, {Name: "x", Ports: []corev1.ContainerPort{{ContainerPort: 8080}}}}, - }}}}, - }, - { - name: "jsonPatch invalid CEL initializer field", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/spec/template/spec/containers/-", - value: Object.spec.template.spec.containers{ - name: "x", - ports: [Object.spec.template.spec.containers.ports{containerPortZ: 8080}] - } - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedErr: "strict decoding error: unknown field \"spec.template.spec.containers[1].ports[0].containerPortZ\"", - }, - { - name: "jsonPatch invalid CEL initializer type", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/spec/template/spec/containers/-", - value: Object.spec.template.spec.containers{ - name: "x", - ports: [Object.spec.template.spec.containers.portsZ{containerPort: 8080}] - } - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "a"}}, - }}}}, - expectedErr: " mismatch: unexpected type name \"Object.spec.template.spec.containers.portsZ\", expected \"Object.spec.template.spec.containers.ports\", which matches field name path from root Object type", - }, - { - name: "jsonPatch add map entry by key and value", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{op: "add", path: "/spec", value: Object.spec{selector: Object.spec.selector{}, replicas: 10}} - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Selector: &metav1.LabelSelector{}, Replicas: ptr.To[int32](10)}}, - }, - { - name: "JSONPatch patch type has field access", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/metadata/labels", - value: { - "op": JSONPatch{op: "opValue"}.op, - "path": JSONPatch{path: "pathValue"}.path, - "from": JSONPatch{from: "fromValue"}.from, - "value": string(JSONPatch{value: "valueValue"}.value), - } - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ - "op": "opValue", - "path": "pathValue", - "from": "fromValue", - "value": "valueValue", - }}}, - }, - { - name: "JSONPatch patch type has field testing", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/metadata/labels", - value: { - "op": string(has(JSONPatch{op: "opValue"}.op)), - "path": string(has(JSONPatch{path: "pathValue"}.path)), - "from": string(has(JSONPatch{from: "fromValue"}.from)), - "value": string(has(JSONPatch{value: "valueValue"}.value)), - "op-unset": string(has(JSONPatch{}.op)), - "path-unset": string(has(JSONPatch{}.path)), - "from-unset": string(has(JSONPatch{}.from)), - "value-unset": string(has(JSONPatch{}.value)), - } - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ - "op": "true", - "path": "true", - "from": "true", - "value": "true", - "op-unset": "false", - "path-unset": "false", - "from-unset": "false", - "value-unset": "false", - }}}, - }, - { - name: "JSONPatch patch type equality", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/metadata/labels", - value: { - "empty": string(JSONPatch{} == JSONPatch{}), - "partial": string(JSONPatch{op: "add"} == JSONPatch{op: "add"}), - "same-all": string(JSONPatch{op: "add", path: "path", from: "from", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), - "different-op": string(JSONPatch{op: "add"} == JSONPatch{op: "remove"}), - "different-path": string(JSONPatch{op: "add", path: "x", from: "from", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), - "different-from": string(JSONPatch{op: "add", path: "path", from: "x", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), - "different-value": string(JSONPatch{op: "add", path: "path", from: "from", value: "1"} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), - } - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ - "empty": "true", - "partial": "true", - "same-all": "true", - "different-op": "false", - "different-path": "false", - "different-from": "false", - "different-value": "false", - }}}, - }, - { - name: "JSONPatch key escaping", - policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ - Expression: `[ - JSONPatch{ - op: "add", path: "/metadata/labels", value: {} - }, - JSONPatch{ - op: "add", path: "/metadata/labels/" + jsonpatch.escapeKey("k8s.io/x~y"), value: "true" - } - ]`, - }), - gvr: deploymentGVR, - object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, - expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ - "k8s.io/x~y": "true", - }}}, - }, { name: "applyConfiguration then jsonPatch", policy: mutations(policy("d1"), v1alpha1.Mutation{ @@ -529,92 +108,15 @@ func TestCompilation(t *testing.T) { expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](111)}}, }, { - name: "apply configuration add to listType=map", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - template: Object.spec.template{ - spec: Object.spec.template.spec{ - volumes: [Object.spec.template.spec.volumes{ - name: "y" - }] - } - } - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Volumes: []corev1.Volume{{Name: "x"}}, - }, - }, - }}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Volumes: []corev1.Volume{{Name: "x"}, {Name: "y"}}, - }, - }, - }}, - }, - { - name: "apply configuration update listType=map entry", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - template: Object.spec.template{ - spec: Object.spec.template.spec{ - volumes: [Object.spec.template.spec.volumes{ - name: "y", - hostPath: Object.spec.template.spec.volumes.hostPath{ - path: "a" - } - }] - } - } - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Volumes: []corev1.Volume{{Name: "x"}, {Name: "y"}}, - }, - }, - }}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Volumes: []corev1.Volume{{Name: "x"}, {Name: "y", VolumeSource: corev1.VolumeSource{HostPath: &corev1.HostPathVolumeSource{Path: "a"}}}}, - }, - }, - }}, - }, - { - name: "apply configuration with conditionals", - policy: applyConfigurations(policy("d1"), ` - Object{ - spec: Object.spec{ - replicas: object.spec.replicas % 2 == 0?object.spec.replicas + 1:object.spec.replicas - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](2)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, - }, - { - name: "apply configuration with old object", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - replicas: oldObject.spec.replicas % 2 == 0?oldObject.spec.replicas + 1:oldObject.spec.replicas - } - }`), + name: "jsonPatch with variable", + policy: jsonPatches(variables(policy("d1"), v1alpha1.Variable{Name: "desired", Expression: "10"}), v1alpha1.JSONPatch{ + Expression: `[ + JSONPatch{op: "replace", path: "/spec/replicas", value: variables.desired + 1}, + ]`, + }), gvr: deploymentGVR, object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - oldObject: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](2)}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](11)}}, }, { name: "apply configuration with variable", @@ -641,95 +143,6 @@ func TestCompilation(t *testing.T) { object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](100)}}, }, - { - name: "complex apply configuration initialization", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - replicas: 1, - template: Object.spec.template{ - metadata: Object.spec.template.metadata{ - labels: {"app": "nginx"} - }, - spec: Object.spec.template.spec{ - containers: [Object.spec.template.spec.containers{ - name: "nginx", - image: "nginx:1.14.2", - ports: [Object.spec.template.spec.containers.ports{ - containerPort: 80 - }], - resources: Object.spec.template.spec.containers.resources{ - limits: {"cpu": "128M"}, - } - }] - } - } - } - }`), - - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, - expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ - Replicas: ptr.To[int32](1), - Template: corev1.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"app": "nginx"}, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "nginx", - Image: "nginx:1.14.2", - Ports: []corev1.ContainerPort{ - {ContainerPort: 80}, - }, - Resources: corev1.ResourceRequirements{ - Limits: corev1.ResourceList{corev1.ResourceName("cpu"): resource.MustParse("128M")}, - }, - }}, - }, - }, - }}, - }, - { - name: "apply configuration with invalid type name", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.specx{ - replicas: 1 - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedErr: "type mismatch: unexpected type name \"Object.specx\", expected \"Object.spec\", which matches field name path from root Object type", - }, - { - name: "apply configuration with invalid field name", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - replicasx: 1 - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedErr: "error applying patch: failed to convert patch object to typed object: .spec.replicasx: field not declared in schema", - }, - { - name: "apply configuration with invalid return type", - policy: applyConfigurations(policy("d1"), - `"I'm a teapot!"`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedErr: "must evaluate to Object but got string", - }, - { - name: "apply configuration with invalid initializer return type", - policy: applyConfigurations(policy("d1"), - `Object.spec.metadata{}`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedErr: "must evaluate to Object but got Object.spec.metadata", - }, { name: "jsonPatch with excessive cost", policy: jsonPatches(variables(policy("d1"), v1alpha1.Variable{Name: "list", Expression: "[0,1,2,3,4,5,6,7,8,9]"}), v1alpha1.JSONPatch{ @@ -793,20 +206,6 @@ func TestCompilation(t *testing.T) { object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](10)}}, }, - { - name: "apply configuration with change to atomic", - policy: applyConfigurations(policy("d1"), - `Object{ - spec: Object.spec{ - selector: Object.spec.selector{ - matchLabels: {"l": "v"} - } - } - }`), - gvr: deploymentGVR, - object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, - expectedErr: "error applying patch: invalid ApplyConfiguration: may not mutate atomic arrays, maps or structs: .spec.selector", - }, { name: "object type has field access", policy: jsonPatches(policy("d1"), v1alpha1.JSONPatch{ @@ -901,10 +300,18 @@ func TestCompilation(t *testing.T) { } ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + t.Cleanup(cancel) tcManager := patch.NewTypeConverterManager(nil, openapitest.NewEmbeddedFileClient()) go tcManager.Run(ctx) + err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, time.Second, true, func(context.Context) (done bool, err error) { + converter := tcManager.GetTypeConverter(deploymentGVK) + return converter != nil, nil + }) + if err != nil { + t.Fatal(err) + } + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var gvk schema.GroupVersionKind @@ -939,7 +346,7 @@ func TestCompilation(t *testing.T) { for _, patcher := range policyEvaluator.Mutators { attrs := admission.NewAttributesRecord(obj, tc.oldObject, gvk, - metaAccessor.GetName(), metaAccessor.GetNamespace(), tc.gvr, + metaAccessor.GetNamespace(), metaAccessor.GetName(), tc.gvr, "", admission.Create, &metav1.CreateOptions{}, false, nil) vAttrs := &admission.VersionedAttributes{ Attributes: attrs, @@ -1038,6 +445,11 @@ func mutations(policy *v1alpha1.MutatingAdmissionPolicy, mutations ...v1alpha1.M return policy } +func matchConstraints(policy *v1alpha1.MutatingAdmissionPolicy, matchConstraints *v1alpha1.MatchResources) *v1alpha1.MutatingAdmissionPolicy { + policy.Spec.MatchConstraints = matchConstraints + return policy +} + type fakeAuthorizer struct{} func (f fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go index d8d0953c22e..832682552b7 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher.go @@ -42,9 +42,8 @@ import ( func NewDispatcher(a authorizer.Authorizer, m *matching.Matcher, tcm patch.TypeConverterManager) generic.Dispatcher[PolicyHook] { res := &dispatcher{ - matcher: m, - authz: a, - //!TODO: pass in static type converter to reduce network calls + matcher: m, + authz: a, typeConverterManager: tcm, } res.Dispatcher = generic.NewPolicyDispatcher[*Policy, *PolicyBinding, PolicyEvaluator]( @@ -138,7 +137,7 @@ func (d *dispatcher) dispatchInvocations( // This would be a bug. The compiler should always return exactly as // many evaluators as there are mutations return nil, k8serrors.NewInternalError(fmt.Errorf("expected %v compiled evaluators for policy %v, got %v", - invocation.Policy.Name, len(invocation.Policy.Spec.Mutations), len(invocation.Evaluator.Mutators))) + len(invocation.Policy.Spec.Mutations), invocation.Policy.Name, len(invocation.Evaluator.Mutators))) } versionedAttr, err := versionedAttributes.VersionedAttribute(invocation.Kind) @@ -152,6 +151,7 @@ func (d *dispatcher) dispatchInvocations( matchResults := invocation.Evaluator.Matcher.Match(ctx, versionedAttr, invocation.Param, authz) if matchResults.Error != nil { addConfigError(matchResults.Error, invocation, metav1.StatusReasonInvalid) + continue } // if preconditions are not met, then skip mutations diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher_test.go new file mode 100644 index 00000000000..443e2879a6d --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/dispatcher_test.go @@ -0,0 +1,675 @@ +/* +Copyright 2024 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 mutating + +import ( + "context" + "github.com/google/go-cmp/cmp" + "testing" + "time" + + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + "k8s.io/api/admissionregistration/v1alpha1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/policy/generic" + "k8s.io/apiserver/pkg/admission/plugin/policy/matching" + "k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/openapi/openapitest" + "k8s.io/utils/ptr" +) + +func TestDispatcher(t *testing.T) { + deploymentGVK := schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} + deploymentGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + testCases := []struct { + name string + object, oldObject runtime.Object + gvk schema.GroupVersionKind + gvr schema.GroupVersionResource + params []runtime.Object // All params are expected to be ConfigMap for this test. + policyHooks []PolicyHook + expect runtime.Object + }{ + { + name: "simple patch", + gvk: deploymentGVK, + gvr: deploymentGVR, + object: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](1), + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{ + { + Policy: mutations(matchConstraints(policy("policy1"), &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }), v1alpha1.Mutation{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + spec: Object.spec{ + replicas: object.spec.replicas + 100 + } + }`, + }}), + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy1", + }, + }}, + }, + }, + expect: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](101), + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + }, + { + name: "with param", + gvk: deploymentGVK, + gvr: deploymentGVR, + object: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](1), + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + params: []runtime.Object{ + &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "default", + }, + Data: map[string]string{ + "key": "10", + }, + }, + }, + policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{ + { + Policy: paramKind(mutations(matchConstraints(policy("policy1"), &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }}), + v1alpha1.Mutation{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + spec: Object.spec{ + replicas: object.spec.replicas + int(params.data['key']) + } + }`, + }}), + &v1alpha1.ParamKind{ + APIVersion: "v1", + Kind: "ConfigMap", + }), + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy1", + ParamRef: &v1alpha1.ParamRef{Name: "cm1", Namespace: "default"}, + }, + }}, + }, + }, + expect: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](11), + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + }, + { + name: "both policies reinvoked", + gvk: deploymentGVK, + gvr: deploymentGVR, + object: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{ + { + Policy: mutations(matchConstraints(policy("policy1"), &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }), v1alpha1.Mutation{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"policy1": string(int(object.?metadata.labels["count"].orValue("1")) + 1)} + } + }`, + }}), + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy1", + }, + }}, + }, + { + Policy: mutations(matchConstraints(policy("policy2"), &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }), v1alpha1.Mutation{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"policy2": string(int(object.?metadata.labels["count"].orValue("1")) + 1)} + } + }`, + }}), + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy2", + }, + }}, + }, + }, + expect: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + Labels: map[string]string{ + "policy1": "2", + "policy2": "2", + }, + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + }, + { + name: "1st policy sets match condition that 2nd policy matches", + gvk: deploymentGVK, + gvr: deploymentGVR, + object: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{ + { + Policy: &v1alpha1.MutatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policy1", + }, + Spec: v1alpha1.MutatingAdmissionPolicySpec{ + MatchConstraints: &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }, + Mutations: []v1alpha1.Mutation{{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"environment": "production"} + } + }`}}, + }, + }, + }, + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy1", + }, + }}, + }, + { + Policy: &v1alpha1.MutatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policy2", + }, + Spec: v1alpha1.MutatingAdmissionPolicySpec{ + MatchConstraints: &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }, + MatchConditions: []v1alpha1.MatchCondition{ + { + Name: "prodonly", + Expression: `object.?metadata.labels["environment"].orValue("") == "production"`, + }, + }, + Mutations: []v1alpha1.Mutation{{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"policy1invoked": "true"} + } + }`}}, + }, + }, + }, + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy2", + }, + }}, + }, + }, + expect: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + Labels: map[string]string{ + "environment": "production", + "policy1invoked": "true", + }, + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + }, + { + // TODO: This behavior pre-exists with webhook match conditions but should be reconsidered + name: "1st policy still does not match after 2nd policy sets match condition", + gvk: deploymentGVK, + gvr: deploymentGVR, + object: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + policyHooks: []generic.PolicyHook[*Policy, *PolicyBinding, PolicyEvaluator]{ + { + Policy: &v1alpha1.MutatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policy1", + }, + Spec: v1alpha1.MutatingAdmissionPolicySpec{ + MatchConstraints: &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }, + MatchConditions: []v1alpha1.MatchCondition{ + { + Name: "prodonly", + Expression: `object.?metadata.labels["environment"].orValue("") == "production"`, + }, + }, + Mutations: []v1alpha1.Mutation{{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"policy1invoked": "true"} + } + }`}}, + }, + }, + }, + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy1", + }, + }}, + }, + { + Policy: &v1alpha1.MutatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "policy2", + }, + Spec: v1alpha1.MutatingAdmissionPolicySpec{ + MatchConstraints: &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + ResourceRules: []v1alpha1.NamedRuleWithOperations{ + { + RuleWithOperations: v1alpha1.RuleWithOperations{ + Rule: v1alpha1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }, + Operations: []admissionregistrationv1.OperationType{"*"}, + }, + }, + }, + }, + Mutations: []v1alpha1.Mutation{{ + PatchType: v1alpha1.PatchTypeApplyConfiguration, + ApplyConfiguration: &v1alpha1.ApplyConfiguration{ + Expression: `Object{ + metadata: Object.metadata{ + labels: {"environment": "production"} + } + }`}}, + }, + }, + }, + Bindings: []*PolicyBinding{{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy2", + }, + }}, + }, + }, + expect: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "d1", + Namespace: "default", + Labels: map[string]string{ + "environment": "production", + }, + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + }, + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + tcManager := patch.NewTypeConverterManager(nil, openapitest.NewEmbeddedFileClient()) + go tcManager.Run(ctx) + + err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, time.Second, true, func(context.Context) (done bool, err error) { + converter := tcManager.GetTypeConverter(deploymentGVK) + return converter != nil, nil + }) + if err != nil { + t.Fatal(err) + } + + scheme := runtime.NewScheme() + err = appsv1.AddToScheme(scheme) + if err != nil { + t.Fatal(err) + } + err = corev1.AddToScheme(scheme) + if err != nil { + t.Fatal(err) + } + + objectInterfaces := admission.NewObjectInterfacesFromScheme(scheme) + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client := fake.NewClientset(tc.params...) + + // always include default namespace + err := client.Tracker().Add(&corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: corev1.NamespaceSpec{}, + }) + if err != nil { + t.Fatal(err) + } + + informerFactory := informers.NewSharedInformerFactory(client, 0) + matcher := matching.NewMatcher(informerFactory.Core().V1().Namespaces().Lister(), client) + paramInformer, err := informerFactory.ForResource(schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}) + if err != nil { + t.Fatal(err) + } + informerFactory.WaitForCacheSync(ctx.Done()) + informerFactory.Start(ctx.Done()) + for i, h := range tc.policyHooks { + tc.policyHooks[i].ParamInformer = paramInformer + tc.policyHooks[i].ParamScope = testParamScope{} + tc.policyHooks[i].Evaluator = compilePolicy(h.Policy) + } + + dispatcher := NewDispatcher(fakeAuthorizer{}, matcher, tcManager) + err = dispatcher.Start(ctx) + if err != nil { + t.Fatalf("error starting dispatcher: %v", err) + } + + metaAccessor, err := meta.Accessor(tc.object) + if err != nil { + t.Fatal(err) + } + + attrs := admission.NewAttributesRecord(tc.object, tc.oldObject, tc.gvk, + metaAccessor.GetNamespace(), metaAccessor.GetName(), tc.gvr, + "", admission.Create, &metav1.CreateOptions{}, false, nil) + vAttrs := &admission.VersionedAttributes{ + Attributes: attrs, + VersionedKind: tc.gvk, + VersionedObject: tc.object, + VersionedOldObject: tc.oldObject, + } + + err = dispatcher.Dispatch(ctx, vAttrs, objectInterfaces, tc.policyHooks) + if err != nil { + t.Fatalf("error dispatching policy hooks: %v", err) + } + + obj := vAttrs.VersionedObject + if !equality.Semantic.DeepEqual(obj, tc.expect) { + t.Errorf("unexpected result, got diff:\n%s\n", cmp.Diff(tc.expect, obj)) + } + }) + } +} + +type testParamScope struct{} + +func (t testParamScope) Name() meta.RESTScopeName { + return meta.RESTScopeNameNamespace +} + +var _ meta.RESTScope = testParamScope{} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/interface.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/interface.go deleted file mode 100644 index bf196461b0f..00000000000 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/interface.go +++ /dev/null @@ -1,60 +0,0 @@ -/* -Copyright 2024 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 mutating - -import ( - celgo "github.com/google/cel-go/cel" - celtypes "github.com/google/cel-go/common/types" - - "k8s.io/apiserver/pkg/admission/plugin/cel" -) - -var _ cel.ExpressionAccessor = &ApplyConfigurationCondition{} - -// ApplyConfigurationCondition contains the inputs needed to compile and evaluate a cel expression -// that returns an apply configuration -type ApplyConfigurationCondition struct { - Expression string -} - -func (v *ApplyConfigurationCondition) GetExpression() string { - return v.Expression -} - -func (v *ApplyConfigurationCondition) ReturnTypes() []*celgo.Type { - return []*celgo.Type{applyConfigObjectType} -} - -var applyConfigObjectType = celtypes.NewObjectType("Object") - -var _ cel.ExpressionAccessor = &JSONPatchCondition{} - -// JSONPatchCondition contains the inputs needed to compile and evaluate a cel expression -// that returns a JSON patch value. -type JSONPatchCondition struct { - Expression string -} - -func (v *JSONPatchCondition) GetExpression() string { - return v.Expression -} - -func (v *JSONPatchCondition) ReturnTypes() []*celgo.Type { - return []*celgo.Type{celgo.ListType(jsonPatchType)} -} - -var jsonPatchType = celtypes.NewObjectType("JSONPatch") diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/interface.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/interface.go index d00d9837316..d717adc2997 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/interface.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/interface.go @@ -29,6 +29,8 @@ import ( // Patcher provides a patch function to perform a mutation to an object in the admission chain. type Patcher interface { + // Patch returns a copy of the object in the request, modified to change specified by the patch. + // The original object in the request MUST NOT be modified in-place. Patch(ctx context.Context, request Request, runtimeCELCostBudget int64) (runtime.Object, error) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch.go index b5cf919ef16..26f73dd3415 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch.go @@ -21,6 +21,7 @@ import ( gojson "encoding/json" "errors" "fmt" + celgo "github.com/google/cel-go/cel" "reflect" "strconv" @@ -41,6 +42,24 @@ import ( pointer "k8s.io/utils/ptr" ) +// JSONPatchCondition contains the inputs needed to compile and evaluate a cel expression +// that returns a JSON patch value. +type JSONPatchCondition struct { + Expression string +} + +var _ plugincel.ExpressionAccessor = &JSONPatchCondition{} + +func (v *JSONPatchCondition) GetExpression() string { + return v.Expression +} + +func (v *JSONPatchCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.ListType(jsonPatchType)} +} + +var jsonPatchType = types.NewObjectType("JSONPatch") + // NewJSONPatcher creates a patcher that performs a JSON Patch mutation. func NewJSONPatcher(patchEvaluator plugincel.MutatingEvaluator) Patcher { return &jsonPatcher{patchEvaluator} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch_test.go index 58c4a55b838..c01b2d3cf50 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/json_patch_test.go @@ -1 +1,486 @@ +/* +Copyright 2024 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 patch + +import ( + "context" + "github.com/google/go-cmp/cmp" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/cel" + celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/cel/environment" + "k8s.io/utils/ptr" +) + +func TestJSONPatch(t *testing.T) { + deploymentGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + tests := []struct { + name string + expression string + gvr schema.GroupVersionResource + object, oldObject runtime.Object + expectedResult runtime.Object + expectedErr string + }{ + { + name: "jsonPatch with false test operation", + expression: `[ + JSONPatch{op: "test", path: "/spec/replicas", value: 100}, + JSONPatch{op: "replace", path: "/spec/replicas", value: 3}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + }, + { + name: "jsonPatch with false test operation", + expression: `[ + JSONPatch{op: "test", path: "/spec/replicas", value: 100}, + JSONPatch{op: "replace", path: "/spec/replicas", value: 3}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + }, + { + name: "jsonPatch with true test operation", + expression: `[ + JSONPatch{op: "test", path: "/spec/replicas", value: 1}, + JSONPatch{op: "replace", path: "/spec/replicas", value: 3}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, + }, + { + name: "jsonPatch remove to unset field", + expression: `[ + JSONPatch{op: "remove", path: "/spec/replicas"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch remove map entry by key", + expression: `[ + JSONPatch{op: "remove", path: "/metadata/labels/y"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1", "y": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch remove element in list", + expression: `[ + JSONPatch{op: "remove", path: "/spec/template/spec/containers/1"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "c"}}, + }}}}, + }, + { + name: "jsonPatch copy map entry by key", + expression: `[ + JSONPatch{op: "copy", from: "/metadata/labels/x", path: "/metadata/labels/y"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1", "y": "1"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch copy first element to end of list", + expression: `[ + JSONPatch{op: "copy", from: "/spec/template/spec/containers/0", path: "/spec/template/spec/containers/-"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}, {Name: "a"}}, + }}}}, + }, + { + name: "jsonPatch move map entry by key", + expression: `[ + JSONPatch{op: "move", from: "/metadata/labels/x", path: "/metadata/labels/y"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch move first element to end of list", + expression: `[ + JSONPatch{op: "move", from: "/spec/template/spec/containers/0", path: "/spec/template/spec/containers/-"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "b"}, {Name: "c"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "b"}, {Name: "c"}, {Name: "a"}}, + }}}}, + }, + { + name: "jsonPatch add map entry by key and value", + expression: `[ + JSONPatch{op: "add", path: "/metadata/labels/x", value: "2"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1", "x": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch add map value to field", + expression: `[ + JSONPatch{op: "add", path: "/metadata/labels", value: {"y": "2"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch add map to existing map", // performs a replacement + expression: `[ + JSONPatch{op: "add", path: "/metadata/labels", value: {"y": "2"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch add to start of list", + expression: `[ + JSONPatch{op: "add", path: "/spec/template/spec/containers/0", value: {"name": "x"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "x"}, {Name: "a"}}, + }}}}, + }, + { + name: "jsonPatch add to end of list", + expression: `[ + JSONPatch{op: "add", path: "/spec/template/spec/containers/-", value: {"name": "x"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "x"}}, + }}}}, + }, + { + name: "jsonPatch replace key in map", + expression: `[ + JSONPatch{op: "replace", path: "/metadata/labels/x", value: "2"}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "1", "x": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch replace map value of unset field", // adds the field value + expression: `[ + JSONPatch{op: "replace", path: "/metadata/labels", value: {"y": "2"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch replace map value of set field", + expression: `[ + JSONPatch{op: "replace", path: "/metadata/labels", value: {"y": "2"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"x": "1"}}, Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"y": "2"}}, Spec: appsv1.DeploymentSpec{}}, + }, + { + name: "jsonPatch replace first element in list", + expression: `[ + JSONPatch{op: "replace", path: "/spec/template/spec/containers/0", value: {"name": "x"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "x"}}, + }}}}, + }, + { + name: "jsonPatch add map entry by key and value", + expression: `[ + JSONPatch{op: "add", path: "/spec", value: Object.spec{selector: Object.spec.selector{}, replicas: 10}} + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Selector: &metav1.LabelSelector{}, Replicas: ptr.To[int32](10)}}, + }, + { + name: "JSONPatch patch type has field access", + expression: `[ + JSONPatch{ + op: "add", path: "/metadata/labels", + value: { + "op": JSONPatch{op: "opValue"}.op, + "path": JSONPatch{path: "pathValue"}.path, + "from": JSONPatch{from: "fromValue"}.from, + "value": string(JSONPatch{value: "valueValue"}.value), + } + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "op": "opValue", + "path": "pathValue", + "from": "fromValue", + "value": "valueValue", + }}}, + }, + { + name: "JSONPatch patch type has field testing", + expression: `[ + JSONPatch{ + op: "add", path: "/metadata/labels", + value: { + "op": string(has(JSONPatch{op: "opValue"}.op)), + "path": string(has(JSONPatch{path: "pathValue"}.path)), + "from": string(has(JSONPatch{from: "fromValue"}.from)), + "value": string(has(JSONPatch{value: "valueValue"}.value)), + "op-unset": string(has(JSONPatch{}.op)), + "path-unset": string(has(JSONPatch{}.path)), + "from-unset": string(has(JSONPatch{}.from)), + "value-unset": string(has(JSONPatch{}.value)), + } + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "op": "true", + "path": "true", + "from": "true", + "value": "true", + "op-unset": "false", + "path-unset": "false", + "from-unset": "false", + "value-unset": "false", + }}}, + }, + { + name: "JSONPatch patch type equality", + expression: `[ + JSONPatch{ + op: "add", path: "/metadata/labels", + value: { + "empty": string(JSONPatch{} == JSONPatch{}), + "partial": string(JSONPatch{op: "add"} == JSONPatch{op: "add"}), + "same-all": string(JSONPatch{op: "add", path: "path", from: "from", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), + "different-op": string(JSONPatch{op: "add"} == JSONPatch{op: "remove"}), + "different-path": string(JSONPatch{op: "add", path: "x", from: "from", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), + "different-from": string(JSONPatch{op: "add", path: "path", from: "x", value: 1} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), + "different-value": string(JSONPatch{op: "add", path: "path", from: "from", value: "1"} == JSONPatch{op: "add", path: "path", from: "from", value: 1}), + } + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "empty": "true", + "partial": "true", + "same-all": "true", + "different-op": "false", + "different-path": "false", + "different-from": "false", + "different-value": "false", + }}}, + }, + { + name: "JSONPatch key escaping", + expression: `[ + JSONPatch{ + op: "add", path: "/metadata/labels", value: {} + }, + JSONPatch{ + op: "add", path: "/metadata/labels/" + jsonpatch.escapeKey("k8s.io/x~y"), value: "true" + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}, + expectedResult: &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "k8s.io/x~y": "true", + }}}, + }, + { + name: "jsonPatch with CEL initializer", + expression: `[ + JSONPatch{op: "add", path: "/spec/template/spec/containers/-", value: Object.spec.template.spec.containers{ + name: "x", + ports: [Object.spec.template.spec.containers.ports{containerPort: 8080}], + } + }, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "x", Ports: []corev1.ContainerPort{{ContainerPort: 8080}}}}, + }}}}, + }, + { + name: "jsonPatch invalid CEL initializer field", + expression: `[ + JSONPatch{ + op: "add", path: "/spec/template/spec/containers/-", + value: Object.spec.template.spec.containers{ + name: "x", + ports: [Object.spec.template.spec.containers.ports{containerPortZ: 8080}] + } + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedErr: "strict decoding error: unknown field \"spec.template.spec.containers[1].ports[0].containerPortZ\"", + }, + { + name: "jsonPatch invalid CEL initializer type", + expression: `[ + JSONPatch{ + op: "add", path: "/spec/template/spec/containers/-", + value: Object.spec.template.spec.containers{ + name: "x", + ports: [Object.spec.template.spec.containers.portsZ{containerPort: 8080}] + } + } + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedErr: " mismatch: unexpected type name \"Object.spec.template.spec.containers.portsZ\", expected \"Object.spec.template.spec.containers.ports\", which matches field name path from root Object type", + }, + { + name: "jsonPatch replace end of list with - not allowed", + expression: `[ + JSONPatch{op: "replace", path: "/spec/template/spec/containers/-", value: {"name": "x"}}, + ]`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}}, + }}}}, + expectedErr: "JSON Patch: replace operation does not apply: doc is missing key: /spec/template/spec/containers/-: missing value", + }, + } + + compiler, err := cel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) + + if err != nil { + t.Fatal(err) + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + accessor := &JSONPatchCondition{Expression: tc.expression} + compileResult := compiler.CompileMutatingEvaluator(accessor, cel.OptionalVariableDeclarations{StrictCost: true, HasPatchTypes: true}, environment.StoredExpressions) + + patcher := jsonPatcher{PatchEvaluator: compileResult} + + scheme := runtime.NewScheme() + err := appsv1.AddToScheme(scheme) + if err != nil { + t.Fatal(err) + } + + var gvk schema.GroupVersionKind + gvks, _, err := scheme.ObjectKinds(tc.object) + if err != nil { + t.Fatal(err) + } + if len(gvks) == 1 { + gvk = gvks[0] + } else { + t.Fatalf("Failed to find gvk for type: %T", tc.object) + } + + metaAccessor, err := meta.Accessor(tc.object) + if err != nil { + t.Fatal(err) + } + + attrs := admission.NewAttributesRecord(tc.object, tc.oldObject, gvk, + metaAccessor.GetNamespace(), metaAccessor.GetName(), tc.gvr, + "", admission.Create, &metav1.CreateOptions{}, false, nil) + vAttrs := &admission.VersionedAttributes{ + Attributes: attrs, + VersionedKind: gvk, + VersionedObject: tc.object, + VersionedOldObject: tc.oldObject, + } + + r := Request{ + MatchedResource: tc.gvr, + VersionedAttributes: vAttrs, + ObjectInterfaces: admission.NewObjectInterfacesFromScheme(scheme), + OptionalVariables: cel.OptionalVariableBindings{}, + } + + got, err := patcher.Patch(context.Background(), r, celconfig.RuntimeCELCostBudget) + if len(tc.expectedErr) > 0 { + if err == nil { + t.Fatalf("expected error: %s", tc.expectedErr) + } else { + if !strings.Contains(err.Error(), tc.expectedErr) { + t.Fatalf("expected error: %s, got: %s", tc.expectedErr, err.Error()) + } + return + } + } + if err != nil && len(tc.expectedErr) == 0 { + t.Fatalf("unexpected error: %v", err) + } + if !equality.Semantic.DeepEqual(tc.expectedResult, got) { + t.Errorf("unexpected result, got diff:\n%s\n", cmp.Diff(tc.expectedResult, got)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd.go index d0e5ca1fa05..cb078b77753 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd.go @@ -20,6 +20,8 @@ import ( "context" "errors" "fmt" + celgo "github.com/google/cel-go/cel" + celtypes "github.com/google/cel-go/common/types" "strings" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" @@ -35,6 +37,24 @@ import ( "k8s.io/apiserver/pkg/cel/mutation/dynamic" ) +// ApplyConfigurationCondition contains the inputs needed to compile and evaluate a cel expression +// that returns an apply configuration +type ApplyConfigurationCondition struct { + Expression string +} + +var _ plugincel.ExpressionAccessor = &ApplyConfigurationCondition{} + +func (v *ApplyConfigurationCondition) GetExpression() string { + return v.Expression +} + +func (v *ApplyConfigurationCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{applyConfigObjectType} +} + +var applyConfigObjectType = celtypes.NewObjectType("Object") + // NewApplyConfigurationPatcher creates a patcher that performs an applyConfiguration mutation. func NewApplyConfigurationPatcher(expressionEvaluator plugincel.MutatingEvaluator) Patcher { return &applyConfigPatcher{expressionEvaluator: expressionEvaluator} @@ -147,6 +167,9 @@ func ApplyStructuredMergeDiff( // validatePatch searches an apply configuration for any arrays, maps or structs elements that are atomic and returns // an error if any are found. +// This prevents accidental removal of fields that can occur when the user intends to modify some +// fields in an atomic type, not realizing that all fields not explicitly set in the new value +// of the atomic will be removed. func validatePatch(v *typed.TypedValue) error { atomics := findAtomics(nil, v.Schema(), v.TypeRef(), v.AsValue()) if len(atomics) > 0 { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd_test.go new file mode 100644 index 00000000000..5fdae77a468 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/smd_test.go @@ -0,0 +1,375 @@ +/* +Copyright 2024 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 patch + +import ( + "context" + "github.com/google/go-cmp/cmp" + "strings" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/cel" + celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/cel/environment" + "k8s.io/client-go/openapi/openapitest" + "k8s.io/utils/ptr" +) + +func TestApplyConfiguration(t *testing.T) { + deploymentGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + deploymentGVK := schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} + tests := []struct { + name string + expression string + gvr schema.GroupVersionResource + object, oldObject runtime.Object + expectedResult runtime.Object + expectedErr string + }{ + { + name: "apply configuration add to listType=map", + expression: `Object{ + spec: Object.spec{ + template: Object.spec.template{ + spec: Object.spec.template.spec{ + volumes: [Object.spec.template.spec.volumes{ + name: "y" + }] + } + } + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}, {Name: "y"}}, + }, + }, + }}, + }, + { + name: "apply configuration add to listType=map", + expression: `Object{ + spec: Object.spec{ + template: Object.spec.template{ + spec: Object.spec.template.spec{ + volumes: [Object.spec.template.spec.volumes{ + name: "y" + }] + } + } + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}}, + }, + }, + }}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}, {Name: "y"}}, + }, + }, + }}, + }, + { + name: "apply configuration update listType=map entry", + expression: `Object{ + spec: Object.spec{ + template: Object.spec.template{ + spec: Object.spec.template.spec{ + volumes: [Object.spec.template.spec.volumes{ + name: "y", + hostPath: Object.spec.template.spec.volumes.hostPath{ + path: "a" + } + }] + } + } + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}, {Name: "y"}}, + }, + }, + }}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{{Name: "x"}, {Name: "y", VolumeSource: corev1.VolumeSource{HostPath: &corev1.HostPathVolumeSource{Path: "a"}}}}, + }, + }, + }}, + }, + { + name: "apply configuration with conditionals", + expression: `Object{ + spec: Object.spec{ + replicas: object.spec.replicas % 2 == 0?object.spec.replicas + 1:object.spec.replicas + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](2)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, + }, + { + name: "apply configuration with old object", + expression: `Object{ + spec: Object.spec{ + replicas: oldObject.spec.replicas % 2 == 0?oldObject.spec.replicas + 1:oldObject.spec.replicas + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + oldObject: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](2)}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](3)}}, + }, + { + name: "complex apply configuration initialization", + expression: `Object{ + spec: Object.spec{ + replicas: 1, + template: Object.spec.template{ + metadata: Object.spec.template.metadata{ + labels: {"app": "nginx"} + }, + spec: Object.spec.template.spec{ + containers: [Object.spec.template.spec.containers{ + name: "nginx", + image: "nginx:1.14.2", + ports: [Object.spec.template.spec.containers.ports{ + containerPort: 80 + }], + resources: Object.spec.template.spec.containers.resources{ + limits: {"cpu": "128M"}, + } + }] + } + } + } + }`, + + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{}}, + expectedResult: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To[int32](1), + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "nginx"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx", + Image: "nginx:1.14.2", + Ports: []corev1.ContainerPort{ + {ContainerPort: 80}, + }, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{corev1.ResourceName("cpu"): resource.MustParse("128M")}, + }, + }}, + }, + }, + }}, + }, + { + name: "apply configuration with change to atomic", + expression: `Object{ + spec: Object.spec{ + selector: Object.spec.selector{ + matchLabels: {"l": "v"} + } + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedErr: "error applying patch: invalid ApplyConfiguration: may not mutate atomic arrays, maps or structs: .spec.selector", + }, + { + name: "apply configuration with invalid type name", + expression: `Object{ + spec: Object.specx{ + replicas: 1 + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedErr: "type mismatch: unexpected type name \"Object.specx\", expected \"Object.spec\", which matches field name path from root Object type", + }, + { + name: "apply configuration with invalid field name", + expression: `Object{ + spec: Object.spec{ + replicasx: 1 + } + }`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedErr: "error applying patch: failed to convert patch object to typed object: .spec.replicasx: field not declared in schema", + }, + { + name: "apply configuration with invalid return type", + expression: `"I'm a teapot!"`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedErr: "must evaluate to Object but got string", + }, + { + name: "apply configuration with invalid initializer return type", + expression: `Object.spec.metadata{}`, + gvr: deploymentGVR, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}}, + expectedErr: "must evaluate to Object but got Object.spec.metadata", + }, + } + + compiler, err := cel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) + if err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + tcManager := NewTypeConverterManager(nil, openapitest.NewEmbeddedFileClient()) + go tcManager.Run(ctx) + + err = wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, time.Second, true, func(context.Context) (done bool, err error) { + converter := tcManager.GetTypeConverter(deploymentGVK) + return converter != nil, nil + }) + if err != nil { + t.Fatal(err) + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + accessor := &ApplyConfigurationCondition{Expression: tc.expression} + compileResult := compiler.CompileMutatingEvaluator(accessor, cel.OptionalVariableDeclarations{StrictCost: true, HasPatchTypes: true}, environment.StoredExpressions) + + patcher := applyConfigPatcher{expressionEvaluator: compileResult} + + scheme := runtime.NewScheme() + err := appsv1.AddToScheme(scheme) + if err != nil { + t.Fatal(err) + } + + var gvk schema.GroupVersionKind + gvks, _, err := scheme.ObjectKinds(tc.object) + if err != nil { + t.Fatal(err) + } + if len(gvks) == 1 { + gvk = gvks[0] + } else { + t.Fatalf("Failed to find gvk for type: %T", tc.object) + } + + metaAccessor, err := meta.Accessor(tc.object) + if err != nil { + t.Fatal(err) + } + + typeAccessor, err := meta.TypeAccessor(tc.object) + if err != nil { + t.Fatal(err) + } + typeAccessor.SetKind(gvk.Kind) + typeAccessor.SetAPIVersion(gvk.GroupVersion().String()) + + attrs := admission.NewAttributesRecord(tc.object, tc.oldObject, gvk, + metaAccessor.GetNamespace(), metaAccessor.GetName(), tc.gvr, + "", admission.Create, &metav1.CreateOptions{}, false, nil) + vAttrs := &admission.VersionedAttributes{ + Attributes: attrs, + VersionedKind: gvk, + VersionedObject: tc.object, + VersionedOldObject: tc.oldObject, + } + + r := Request{ + MatchedResource: tc.gvr, + VersionedAttributes: vAttrs, + ObjectInterfaces: admission.NewObjectInterfacesFromScheme(scheme), + OptionalVariables: cel.OptionalVariableBindings{}, + TypeConverter: tcManager.GetTypeConverter(gvk), + } + + patched, err := patcher.Patch(ctx, r, celconfig.RuntimeCELCostBudget) + if len(tc.expectedErr) > 0 { + if err == nil { + t.Fatalf("expected error: %s", tc.expectedErr) + } else { + if !strings.Contains(err.Error(), tc.expectedErr) { + t.Fatalf("expected error: %s, got: %s", tc.expectedErr, err.Error()) + } + return + } + } + if err != nil && len(tc.expectedErr) == 0 { + t.Fatalf("unexpected error: %v", err) + } + + got, err := runtime.DefaultUnstructuredConverter.ToUnstructured(patched) + if err != nil { + t.Fatal(err) + } + + wantTypeAccessor, err := meta.TypeAccessor(tc.expectedResult) + if err != nil { + t.Fatal(err) + } + wantTypeAccessor.SetKind(gvk.Kind) + wantTypeAccessor.SetAPIVersion(gvk.GroupVersion().String()) + + want, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.expectedResult) + + if err != nil { + t.Fatal(err) + } + if !equality.Semantic.DeepEqual(want, got) { + t.Errorf("unexpected result, got diff:\n%s\n", cmp.Diff(want, got)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter_test.go new file mode 100644 index 00000000000..37bc18e9c7a --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter_test.go @@ -0,0 +1,100 @@ +/* +Copyright 2024 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 patch + +import ( + "context" + "github.com/google/go-cmp/cmp" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/openapi/openapitest" +) + +func TestTypeConverter(t *testing.T) { + deploymentGVK := schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} + tests := []struct { + name string + gvk schema.GroupVersionKind + object runtime.Object + }{ + { + name: "simple round trip", + gvk: deploymentGVK, + object: &appsv1.Deployment{Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "a"}, {Name: "x", Ports: []corev1.ContainerPort{{ContainerPort: 8080}}}}, + }}}}, + }, + } + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + tcManager := NewTypeConverterManager(nil, openapitest.NewEmbeddedFileClient()) + go tcManager.Run(ctx) + + err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, time.Second, true, func(context.Context) (done bool, err error) { + converter := tcManager.GetTypeConverter(deploymentGVK) + return converter != nil, nil + }) + if err != nil { + t.Fatal(err) + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + typeAccessor, err := meta.TypeAccessor(tc.object) + if err != nil { + t.Fatal(err) + } + typeAccessor.SetKind(tc.gvk.Kind) + typeAccessor.SetAPIVersion(tc.gvk.GroupVersion().String()) + + converter := tcManager.GetTypeConverter(tc.gvk) + if converter == nil { + t.Errorf("nil TypeConverter") + } + typedObject, err := converter.ObjectToTyped(tc.object) + if err != nil { + t.Fatal(err) + } + + roundTripped, err := converter.TypedToObject(typedObject) + if err != nil { + t.Fatal(err) + } + got, err := runtime.DefaultUnstructuredConverter.ToUnstructured(roundTripped) + if err != nil { + t.Fatal(err) + } + + want, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.object) + if err != nil { + t.Fatal(err) + } + if !equality.Semantic.DeepEqual(want, got) { + t.Errorf("unexpected result, got diff:\n%s\n", cmp.Diff(want, got)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin.go index fa84539efe3..527bc6a53c0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin.go @@ -53,7 +53,6 @@ func Register(plugins *admission.Plugins) { }) } -// Plugin is an implementation of admission.Interface. type Policy = v1alpha1.MutatingAdmissionPolicy type PolicyBinding = v1alpha1.MutatingAdmissionPolicyBinding type PolicyMutation = v1alpha1.Mutation @@ -80,6 +79,7 @@ type PolicyEvaluator struct { Error error } +// Plugin is an implementation of admission.Interface. type Plugin struct { *generic.Plugin[PolicyHook] } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin_test.go index 884e7dec1e9..9d7652fd04d 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/plugin_test.go @@ -119,6 +119,68 @@ func TestBasicPatch(t *testing.T) { require.Equal(t, expectedAnnotations, testObject.Annotations) } +func TestJSONPatch(t *testing.T) { + patchObj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + "foo": "bar", + }, + }, + "data": map[string]interface{}{ + "myfield": "myvalue", + }, + }, + } + + testContext := setupTest(t, func(p *mutating.Policy) mutating.PolicyEvaluator { + return mutating.PolicyEvaluator{ + Mutators: []patch.Patcher{smdPatcher{patch: patchObj}}, + } + }) + + // Set up a policy and binding that match, no params + require.NoError(t, testContext.UpdateAndWait( + &mutating.Policy{ + ObjectMeta: metav1.ObjectMeta{Name: "policy"}, + Spec: v1alpha1.MutatingAdmissionPolicySpec{ + MatchConstraints: &v1alpha1.MatchResources{ + MatchPolicy: ptr.To(v1alpha1.Equivalent), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + }, + Mutations: []v1alpha1.Mutation{ + { + JSONPatch: &v1alpha1.JSONPatch{ + Expression: "ignored, but required", + }, + PatchType: v1alpha1.PatchTypeApplyConfiguration, + }, + }, + }, + }, + &mutating.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: v1alpha1.MutatingAdmissionPolicyBindingSpec{ + PolicyName: "policy", + }, + }, + )) + + // Show that if we run an object through the policy, it gets the annotation + testObject := &corev1.ConfigMap{} + err := testContext.Dispatch(testObject, nil, admission.Create) + require.NoError(t, err) + require.Equal(t, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"foo": "bar"}, + }, + Data: map[string]string{"myfield": "myvalue"}, + }, testObject) +} + func TestSSAPatch(t *testing.T) { patchObj := &unstructured.Unstructured{ Object: map[string]interface{}{ diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext.go index 4ba030c283d..764ce392789 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2024 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. diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext_test.go new file mode 100644 index 00000000000..bc85bb56b34 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/reinvocationcontext_test.go @@ -0,0 +1,147 @@ +/* +Copyright 2024 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 mutating + +import ( + "github.com/stretchr/testify/assert" + "testing" + + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" +) + +func TestFullReinvocation(t *testing.T) { + key1 := key{PolicyUID: types.NamespacedName{Name: "p1"}, BindingUID: types.NamespacedName{Name: "b1"}} + key2 := key{PolicyUID: types.NamespacedName{Name: "p2"}, BindingUID: types.NamespacedName{Name: "b2"}} + key3 := key{PolicyUID: types.NamespacedName{Name: "p3"}, BindingUID: types.NamespacedName{Name: "b3"}} + + cm1v1 := &v1.ConfigMap{Data: map[string]string{"v": "1"}} + cm1v2 := &v1.ConfigMap{Data: map[string]string{"v": "2"}} + + rc := policyReinvokeContext{} + + // key1 is invoked and it updates the configmap + rc.SetLastPolicyInvocationOutput(cm1v1) + rc.RequireReinvokingPreviouslyInvokedPlugins() + rc.AddReinvocablePolicyToPreviouslyInvoked(key1) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v2)) + + // key2 is invoked and it updates the configmap + rc.SetLastPolicyInvocationOutput(cm1v2) + rc.RequireReinvokingPreviouslyInvokedPlugins() + rc.AddReinvocablePolicyToPreviouslyInvoked(key2) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + + // key3 is invoked but it does not change anything + rc.AddReinvocablePolicyToPreviouslyInvoked(key3) + + assert.False(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v2)) + + // key1 is reinvoked + assert.True(t, rc.ShouldReinvoke(key1)) + rc.AddReinvocablePolicyToPreviouslyInvoked(key1) + rc.SetLastPolicyInvocationOutput(cm1v1) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v2)) + rc.RequireReinvokingPreviouslyInvokedPlugins() + + // key2 is reinvoked + assert.True(t, rc.ShouldReinvoke(key2)) + rc.AddReinvocablePolicyToPreviouslyInvoked(key2) + rc.SetLastPolicyInvocationOutput(cm1v2) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + rc.RequireReinvokingPreviouslyInvokedPlugins() + + // key3 is reinvoked, because the reinvocations have changed the resource + assert.True(t, rc.ShouldReinvoke(key3)) +} + +func TestPartialReinvocation(t *testing.T) { + key1 := key{PolicyUID: types.NamespacedName{Name: "p1"}, BindingUID: types.NamespacedName{Name: "b1"}} + key2 := key{PolicyUID: types.NamespacedName{Name: "p2"}, BindingUID: types.NamespacedName{Name: "b2"}} + key3 := key{PolicyUID: types.NamespacedName{Name: "p3"}, BindingUID: types.NamespacedName{Name: "b3"}} + + cm1v1 := &v1.ConfigMap{Data: map[string]string{"v": "1"}} + cm1v2 := &v1.ConfigMap{Data: map[string]string{"v": "2"}} + + rc := policyReinvokeContext{} + + // key1 is invoked and it updates the configmap + rc.SetLastPolicyInvocationOutput(cm1v1) + rc.RequireReinvokingPreviouslyInvokedPlugins() + rc.AddReinvocablePolicyToPreviouslyInvoked(key1) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v2)) + + // key2 is invoked and it updates the configmap + rc.SetLastPolicyInvocationOutput(cm1v2) + rc.RequireReinvokingPreviouslyInvokedPlugins() + rc.AddReinvocablePolicyToPreviouslyInvoked(key2) + + assert.True(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + + // key3 is invoked but it does not change anything + rc.AddReinvocablePolicyToPreviouslyInvoked(key3) + + assert.False(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v2)) + + // key1 is reinvoked but does not change anything + assert.True(t, rc.ShouldReinvoke(key1)) + + // key2 is not reinvoked because nothing changed since last invocation + assert.False(t, rc.ShouldReinvoke(key2)) + + // key3 is not reinvoked because nothing changed since last invocation + assert.False(t, rc.ShouldReinvoke(key3)) +} + +func TestNoReinvocation(t *testing.T) { + key1 := key{PolicyUID: types.NamespacedName{Name: "p1"}, BindingUID: types.NamespacedName{Name: "b1"}} + key2 := key{PolicyUID: types.NamespacedName{Name: "p2"}, BindingUID: types.NamespacedName{Name: "b2"}} + key3 := key{PolicyUID: types.NamespacedName{Name: "p3"}, BindingUID: types.NamespacedName{Name: "b3"}} + + cm1v1 := &v1.ConfigMap{Data: map[string]string{"v": "1"}} + + rc := policyReinvokeContext{} + + // key1 is invoked and it updates the configmap + rc.AddReinvocablePolicyToPreviouslyInvoked(key1) + rc.SetLastPolicyInvocationOutput(cm1v1) + + assert.False(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + + // key2 is invoked but does not change anything + rc.AddReinvocablePolicyToPreviouslyInvoked(key2) + rc.SetLastPolicyInvocationOutput(cm1v1) + + assert.False(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + + // key3 is invoked but it does not change anything + rc.AddReinvocablePolicyToPreviouslyInvoked(key3) + rc.SetLastPolicyInvocationOutput(cm1v1) + + assert.False(t, rc.IsOutputChangedSinceLastPolicyInvocation(cm1v1)) + + // no keys are reinvoked + assert.False(t, rc.ShouldReinvoke(key1)) + assert.False(t, rc.ShouldReinvoke(key2)) + assert.False(t, rc.ShouldReinvoke(key3)) + +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/typechecking.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/typechecking.go index aa5a0f29407..192be9621bd 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/typechecking.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/validating/typechecking.go @@ -436,7 +436,7 @@ func buildEnvSet(hasParams bool, hasAuthorizer bool, types typeOverwrite) (*envi ) } -// createVariableOpts creates a slice of ResolverEnvOption +// createVariableOpts creates a slice of EnvOption // that can be used for creating a CEL env containing variables of declType. // declType can be nil, in which case the variables will be of DynType. func createVariableOpts(declType *apiservercel.DeclType, variables ...string) []cel.EnvOption { diff --git a/staging/src/k8s.io/apiserver/pkg/cel/mutation/dynamic/objects.go b/staging/src/k8s.io/apiserver/pkg/cel/mutation/dynamic/objects.go index b00db23de4e..8dd38281b0c 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/mutation/dynamic/objects.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/mutation/dynamic/objects.go @@ -235,11 +235,11 @@ func convertField(value ref.Val) (any, error) { // unstructured maps, as seen in annotations // map keys must be strings if mapOfVal, ok := value.Value().(map[ref.Val]ref.Val); ok { - result := make(map[string]any) + result := make(map[string]any, len(mapOfVal)) for k, v := range mapOfVal { stringKey, ok := k.Value().(string) if !ok { - return nil, fmt.Errorf("map key %q is of type %t, not string", k, k) + return nil, fmt.Errorf("map key %q is of type %T, not string", k, k) } result[stringKey] = v.Value() } diff --git a/staging/src/k8s.io/apiserver/pkg/cel/mutation/jsonpatch.go b/staging/src/k8s.io/apiserver/pkg/cel/mutation/jsonpatch.go index 1cb018db957..1f6d63fa22e 100644 --- a/staging/src/k8s.io/apiserver/pkg/cel/mutation/jsonpatch.go +++ b/staging/src/k8s.io/apiserver/pkg/cel/mutation/jsonpatch.go @@ -121,14 +121,21 @@ func (p *JSONPatchVal) ConvertToType(typeValue ref.Type) ref.Val { } else if typeValue == types.TypeType { return types.NewTypeTypeWithParam(jsonPatchType) } - return types.NewErr("Unsupported type: %s", typeValue.TypeName()) + return types.NewErr("unsupported type: %s", typeValue.TypeName()) } func (p *JSONPatchVal) Equal(other ref.Val) ref.Val { if o, ok := other.(*JSONPatchVal); ok && p != nil && o != nil { - if *p == *o { + if p.Op != o.Op || p.From != o.From || p.Path != o.Path { + return types.False + } + if (p.Val == nil) != (o.Val == nil) { + return types.False + } + if p.Val == nil { return types.True } + return p.Val.Equal(o.Val) } return types.False }