feat(mutate): minimize unmarshals (kyverno#10702)

* feat(mutate): minimize unmarshals Signed-off-by: Khaled Emara <[email protected]> * test(mutate): test type assertion Signed-off-by: Khaled Emara <[email protected]> * chore(codegen): remove unused import Signed-off-by: Khaled Emara <[email protected]> --------- Signed-off-by: Khaled Emara <[email protected]>
MariamFahmy98 · Aug 9, 2024 · 65a43d2 · 65a43d2
1 parent 6e73e85
commit 65a43d2
Show file tree

Hide file tree

Showing 10 changed files with 60 additions and 34 deletions.
diff --git 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
diff --git a/docs/user/crd/index.html b/docs/user/crd/index.html
@@ -1724,9 +1724,7 @@ <h3 id="kyverno.io/v1.ForEachMutation">ForEachMutation
 <td>
 <code>patchStrategicMerge</code><br/>
 <em>
-<a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#json-v1-apiextensions">
-Kubernetes apiextensions/v1.JSON
-</a>
+github.com/kyverno/kyverno/api/kyverno.Any
 </em>
 </td>
 <td>

diff --git a/docs/user/crd/kyverno.v1.html b/docs/user/crd/kyverno.v1.html
@@ -3540,7 +3540,7 @@ <H3 id="kyverno-io-v1-ForEachMutation">ForEachMutation
 
 
 
-              <span style="font-family: monospace">k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON</span>
+              <span style="font-family: monospace">github.com/kyverno/kyverno/api/kyverno.Any</span>
 
 
         </td>

diff --git 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
diff --git 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
@@ -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)
-	if err != nil {
-		return nil, err
-	}
-
-	data, err := variables.SubstituteAll(logger, ctx, jsonObj)
-	if err != nil {
-		return nil, err
-	}
+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
 
-	bytes, err := json.Marshal(data)
+	subedPatchesMap, err := variables.SubstituteAll(logger, ctx, patchesMap)
 	if err != nil {
 		return nil, err
 	}
 
-	var updatedForEach kyvernov1.ForEachMutation
-	if err := json.Unmarshal(bytes, &updatedForEach); 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{}")
 	}
 
-	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
@@ -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
@@ -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")
 			}
Original file line number	Diff line number	Diff line change
Expand Up		@@ -3540,7 +3540,7 @@ <H3 id="kyverno-io-v1-ForEachMutation">ForEachMutation



		<span style="font-family: monospace">k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON</span>
		<span style="font-family: monospace">github.com/kyverno/kyverno/api/kyverno.Any</span>


		</td>
Expand Down