From 65a43d20590fbd5a4c84b15be8c38488d946816e Mon Sep 17 00:00:00 2001 From: Khaled Emara Date: Fri, 9 Aug 2024 14:12:20 +0300 Subject: [PATCH] feat(mutate): minimize unmarshals (#10702) * feat(mutate): minimize unmarshals Signed-off-by: Khaled Emara * test(mutate): test type assertion Signed-off-by: Khaled Emara * chore(codegen): remove unused import Signed-off-by: Khaled Emara --------- Signed-off-by: Khaled Emara --- api/kyverno/v1/common_types.go | 12 ++++--- api/kyverno/v1/zz_generated.deepcopy.go | 3 +- docs/user/crd/index.html | 4 +-- docs/user/crd/kyverno.v1.html | 2 +- go.mod | 1 + .../kyverno/v1/foreachmutation.go | 6 ++-- pkg/engine/handlers/mutation/common.go | 2 +- pkg/engine/mutate/mutation.go | 30 ++++++---------- pkg/engine/mutate/mutation_test.go | 34 +++++++++++++++++++ pkg/policy/mutate/validate.go | 2 +- 10 files changed, 61 insertions(+), 35 deletions(-) diff --git a/api/kyverno/v1/common_types.go b/api/kyverno/v1/common_types.go index 0204faafda..209245e9a4 100644 --- a/api/kyverno/v1/common_types.go +++ b/api/kyverno/v1/common_types.go @@ -418,7 +418,9 @@ type ForEachMutation struct { // See https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/ // and https://kubectl.docs.kubernetes.io/references/kustomize/patchesstrategicmerge/. // +optional - RawPatchStrategicMerge *apiextv1.JSON `json:"patchStrategicMerge,omitempty" yaml:"patchStrategicMerge,omitempty"` + // +kubebuilder:validation:Schemaless + // +kubebuilder:pruning:PreserveUnknownFields + RawPatchStrategicMerge *kyverno.Any `json:"patchStrategicMerge,omitempty" yaml:"patchStrategicMerge,omitempty"` // PatchesJSON6902 is a list of RFC 6902 JSON Patch declarations used to modify resources. // See https://tools.ietf.org/html/rfc6902 and https://kubectl.docs.kubernetes.io/references/kustomize/patchesjson6902/. @@ -439,12 +441,12 @@ func (m *ForEachMutation) GetForEachMutation() []ForEachMutation { return m.ForEachMutation.Items } -func (m *ForEachMutation) GetPatchStrategicMerge() apiextensions.JSON { - return FromJSON(m.RawPatchStrategicMerge) +func (m *ForEachMutation) GetPatchStrategicMerge() any { + return kyverno.FromAny(m.RawPatchStrategicMerge) } -func (m *ForEachMutation) SetPatchStrategicMerge(in apiextensions.JSON) { - m.RawPatchStrategicMerge = ToJSON(in) +func (m *ForEachMutation) SetPatchStrategicMerge(in any) { + m.RawPatchStrategicMerge = kyverno.ToAny(in) } // Validation defines checks to be performed on matching resources. diff --git a/api/kyverno/v1/zz_generated.deepcopy.go b/api/kyverno/v1/zz_generated.deepcopy.go index 11e30252b5..c6feffb85c 100755 --- a/api/kyverno/v1/zz_generated.deepcopy.go +++ b/api/kyverno/v1/zz_generated.deepcopy.go @@ -559,8 +559,7 @@ func (in *ForEachMutation) DeepCopyInto(out *ForEachMutation) { } if in.RawPatchStrategicMerge != nil { in, out := &in.RawPatchStrategicMerge, &out.RawPatchStrategicMerge - *out = new(apiextensionsv1.JSON) - (*in).DeepCopyInto(*out) + *out = (*in).DeepCopy() } if in.ForEachMutation != nil { in, out := &in.ForEachMutation, &out.ForEachMutation diff --git a/docs/user/crd/index.html b/docs/user/crd/index.html index 17ff508046..98908837ed 100644 --- a/docs/user/crd/index.html +++ b/docs/user/crd/index.html @@ -1724,9 +1724,7 @@ See: https://k patchStrategicMerge
- -Kubernetes apiextensions/v1.JSON - +github.com/kyverno/kyverno/api/kyverno.Any diff --git a/docs/user/crd/kyverno.v1.html b/docs/user/crd/kyverno.v1.html index 1baa341e88..433baef7a5 100644 --- a/docs/user/crd/kyverno.v1.html +++ b/docs/user/crd/kyverno.v1.html @@ -3540,7 +3540,7 @@ See: https://kyverno.io/docs/writing-policies/preconditions/

- k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON + github.com/kyverno/kyverno/api/kyverno.Any diff --git a/go.mod b/go.mod index 67ed138691..a3e98a9c8e 100644 --- a/go.mod +++ b/go.mod @@ -331,6 +331,7 @@ require ( github.com/spf13/viper v1.19.0 // indirect github.com/spiffe/go-spiffe/v2 v2.2.0 // indirect github.com/stoewer/go-strcase v1.3.0 // indirect + github.com/stretchr/objx v0.5.2 // indirect github.com/subosito/gotenv v1.6.0 // indirect github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d // indirect github.com/tchap/go-patricia/v2 v2.3.1 // indirect diff --git a/pkg/client/applyconfigurations/kyverno/v1/foreachmutation.go b/pkg/client/applyconfigurations/kyverno/v1/foreachmutation.go index d6f56a6696..ae0747fc02 100644 --- a/pkg/client/applyconfigurations/kyverno/v1/foreachmutation.go +++ b/pkg/client/applyconfigurations/kyverno/v1/foreachmutation.go @@ -19,8 +19,8 @@ limitations under the License. package v1 import ( + kyverno "github.com/kyverno/kyverno/api/kyverno" v1 "github.com/kyverno/kyverno/api/kyverno/v1" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) // ForEachMutationApplyConfiguration represents an declarative configuration of the ForEachMutation type for use @@ -30,7 +30,7 @@ type ForEachMutationApplyConfiguration struct { Order *v1.ForeachOrder `json:"order,omitempty"` Context []ContextEntryApplyConfiguration `json:"context,omitempty"` AnyAllConditions *AnyAllConditionsApplyConfiguration `json:"preconditions,omitempty"` - RawPatchStrategicMerge *apiextensionsv1.JSON `json:"patchStrategicMerge,omitempty"` + RawPatchStrategicMerge *kyverno.Any `json:"patchStrategicMerge,omitempty"` PatchesJSON6902 *string `json:"patchesJson6902,omitempty"` ForEachMutation *v1.ForEachMutationWrapper `json:"foreach,omitempty"` } @@ -81,7 +81,7 @@ func (b *ForEachMutationApplyConfiguration) WithAnyAllConditions(value *AnyAllCo // WithRawPatchStrategicMerge sets the RawPatchStrategicMerge field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the RawPatchStrategicMerge field is set to the value of the last call. -func (b *ForEachMutationApplyConfiguration) WithRawPatchStrategicMerge(value apiextensionsv1.JSON) *ForEachMutationApplyConfiguration { +func (b *ForEachMutationApplyConfiguration) WithRawPatchStrategicMerge(value kyverno.Any) *ForEachMutationApplyConfiguration { b.RawPatchStrategicMerge = &value return b } diff --git a/pkg/engine/handlers/mutation/common.go b/pkg/engine/handlers/mutation/common.go index 5ac473daca..a6862f8e71 100644 --- a/pkg/engine/handlers/mutation/common.go +++ b/pkg/engine/handlers/mutation/common.go @@ -69,7 +69,7 @@ func (f *forEachMutator) mutateElements(ctx context.Context, foreach kyvernov1.F reverse := false // if it's a patch strategic merge, reverse by default - if foreach.RawPatchStrategicMerge != nil { + if foreach.GetPatchStrategicMerge() != nil { reverse = true } if foreach.Order != nil { diff --git a/pkg/engine/mutate/mutation.go b/pkg/engine/mutate/mutation.go index 2ca2c4421d..9f730430fe 100644 --- a/pkg/engine/mutate/mutation.go +++ b/pkg/engine/mutate/mutation.go @@ -1,7 +1,6 @@ package mutate import ( - "encoding/json" "fmt" "strings" @@ -11,7 +10,6 @@ import ( "github.com/kyverno/kyverno/pkg/engine/context" "github.com/kyverno/kyverno/pkg/engine/mutate/patch" "github.com/kyverno/kyverno/pkg/engine/variables" - datautils "github.com/kyverno/kyverno/pkg/utils/data" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -77,7 +75,7 @@ func ForEach(name string, foreach kyvernov1.ForEachMutation, policyContext engin if err != nil { return NewErrorResponse("variable substitution failed", err) } - patcher := NewPatcher(fe.GetPatchStrategicMerge(), fe.PatchesJSON6902) + patcher := NewPatcher(fe["patchStrategicMerge"], fe["patchesJson6902"].(string)) if patcher == nil { return NewErrorResponse("empty mutate rule", nil) } @@ -101,28 +99,22 @@ func ForEach(name string, foreach kyvernov1.ForEachMutation, policyContext engin return NewResponse(engineapi.RuleStatusPass, *patchedResource, "resource patched") } -func substituteAllInForEach(fe kyvernov1.ForEachMutation, ctx context.Interface, logger logr.Logger) (*kyvernov1.ForEachMutation, error) { - jsonObj, err := datautils.ToMap(fe) +func substituteAllInForEach(fe kyvernov1.ForEachMutation, ctx context.Interface, logger logr.Logger) (map[string]interface{}, error) { + patchesMap := make(map[string]interface{}) + patchesMap["patchStrategicMerge"] = fe.GetPatchStrategicMerge() + patchesMap["patchesJson6902"] = fe.PatchesJSON6902 + + subedPatchesMap, err := variables.SubstituteAll(logger, ctx, patchesMap) if err != nil { return nil, err } - data, err := variables.SubstituteAll(logger, ctx, jsonObj) - if err != nil { - return nil, err + typedMap, ok := subedPatchesMap.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("failed to convert patched map to map[string]interface{}") } - bytes, err := json.Marshal(data) - if err != nil { - return nil, err - } - - var updatedForEach kyvernov1.ForEachMutation - if err := json.Unmarshal(bytes, &updatedForEach); err != nil { - return nil, err - } - - return &updatedForEach, nil + return typedMap, nil } func NewPatcher(strategicMergePatch apiextensions.JSON, jsonPatch string) patch.Patcher { diff --git a/pkg/engine/mutate/mutation_test.go b/pkg/engine/mutate/mutation_test.go index ca4ef1c5d4..a12a21c559 100644 --- a/pkg/engine/mutate/mutation_test.go +++ b/pkg/engine/mutate/mutation_test.go @@ -7,10 +7,13 @@ import ( "github.com/go-logr/logr" types "github.com/kyverno/kyverno/api/kyverno/v1" + v1 "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/engine/context" "github.com/kyverno/kyverno/pkg/engine/jmespath" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -235,3 +238,34 @@ func TestProcessPatches_RemovePathDoesntExist_NotEmptyResult(t *testing.T) { unstructured.SetNestedField(resource.UnstructuredContent(), "label2Value", "metadata", "labels", "label2") require.Equal(t, resource, patched) } + +type MockContext struct { + context.Interface + mock.Mock +} + +func (m *MockContext) Query(query string) (interface{}, error) { + args := m.Called(query) + return args.Get(0), args.Error(1) +} + +func (m *MockContext) QueryOperation() string { + args := m.Called() + return args.Get(0).(string) +} + +func TestSubstituteAllInForEach_InvalidTypeConversion(t *testing.T) { + ctx := &MockContext{} + // Simulate a scenario where the substitution returns an unexpected type + ctx.On("Query", mock.Anything).Return(true, nil) + ctx.On("QueryOperation").Return("CREATE") + + foreach := v1.ForEachMutation{ + PatchesJSON6902: "string", + } + + fe, err := substituteAllInForEach(foreach, ctx, logr.Discard()) + + assert.NoError(t, err) + assert.IsType(t, "string", fe["patchesJson6902"]) +} diff --git a/pkg/policy/mutate/validate.go b/pkg/policy/mutate/validate.go index e5973bb277..88c311fe03 100644 --- a/pkg/policy/mutate/validate.go +++ b/pkg/policy/mutate/validate.go @@ -55,7 +55,7 @@ func (m *Mutate) validateForEach(tag string, foreach []kyvernov1.ForEachMutation tag = tag + fmt.Sprintf("foreach[%d]", i) fem := fe.GetForEachMutation() if len(fem) > 0 { - if fe.Context != nil || fe.AnyAllConditions != nil || fe.PatchesJSON6902 != "" || fe.RawPatchStrategicMerge != nil { + if fe.Context != nil || fe.AnyAllConditions != nil || fe.PatchesJSON6902 != "" || fe.GetPatchStrategicMerge() != nil { return tag, fmt.Errorf("a nested foreach cannot contain other declarations") }