Substitute vars in map keys (#2344)

* substitute vars in map keys

Signed-off-by: Maxim Goncharenko <goncharenko.maxim@apriorit.com>

* add test for 2316 issue case

Signed-off-by: Maxim Goncharenko <goncharenko.maxim@apriorit.com>

pkg/engine/json-utils/traverse.go

@@ -1,6 +1,7 @@
 package json_utils
 
 import (
+	"fmt"
 	"strconv"
 
 	"github.com/kyverno/kyverno/pkg/engine/common"
@@ -17,13 +18,15 @@ type ActionData struct {
 // JSON element
 type Action func(data *ActionData) (interface{}, error)
 
-// OnlyForLeafs is an action modifier - apply action only for leafs
-func OnlyForLeafs(action Action) Action {
+// OnlyForLeafsAndKeys is an action modifier - apply action only for leafs and map keys
+func OnlyForLeafsAndKeys(action Action) Action {
 	return func(data *ActionData) (interface{}, error) {
-		switch data.Element.(type) {
+		switch typed := data.Element.(type) {
 		case map[string]interface{}, []interface{}: // skip arrays and maps
 			return data.Element, nil
-
+		case Key:
+			data.Element = typed.Key
+			return action(data)
 		default: // leaf detected
 			return action(data)
 		}
@@ -45,6 +48,11 @@ func NewTraversal(document interface{}, action Action) *Traversal {
 	}
 }
 
+// Key type is needed for traversal to specify the key
+type Key struct {
+	Key string
+}
+
 // TraverseJSON performs a traverse of JSON document and applying
 // action for each JSON element
 func (t *Traversal) TraverseJSON() (interface{}, error) {
@@ -66,6 +74,9 @@ func (t *Traversal) traverseJSON(element interface{}, path string) (interface{},
 	case []interface{}:
 		return t.traverseList(common.CopySlice(typed), path)
 
+	case Key:
+		return typed.Key, nil
+
 	default:
 		return element, nil
 	}
@@ -73,12 +84,34 @@ func (t *Traversal) traverseJSON(element interface{}, path string) (interface{},
 
 func (t *Traversal) traverseObject(object map[string]interface{}, path string) (map[string]interface{}, error) {
 	for key, element := range object {
+		newKey, err := t.traverseJSON(Key{key}, path)
+		if err != nil {
+			return nil, err
+		}
+		var newKeyStr string
+		if newKey == nil {
+			newKeyStr = key
+		} else {
+			var ok bool
+			if newKeyStr, ok = newKey.(string); !ok {
+				return nil, fmt.Errorf("expected string after substituting variables in key \"%s\"", key)
+			}
+		}
+
 		value, err := t.traverseJSON(element, path+"/"+key)
 		if err != nil {
 			return nil, err
 		}
+
+		if newKeyStr != key {
+			// key was renamed after var substitution
+			// delete old key
+			object[newKeyStr] = value
+			delete(object, key)
+		} else {
 			object[key] = value
 		}
+	}
 	return object, nil
 }
 

pkg/engine/json-utils/traverse_test.go

@@ -42,12 +42,15 @@ func Test_TraverseLeafsCheckIfTheyHit(t *testing.T) {
 	err := json.Unmarshal(document, &originalJSON)
 	assert.NilError(t, err)
 
-	traversal := NewTraversal(originalJSON, OnlyForLeafs(func(data *ActionData) (interface{}, error) {
-		hitMap[data.Element.(string)]++
+	traversal := NewTraversal(originalJSON, OnlyForLeafsAndKeys(func(data *ActionData) (interface{}, error) {
+		if key, ok := data.Element.(string); ok {
+			hitMap[key]++
+		}
 		return data.Element, nil
 	}))
 
 	_, err = traversal.TraverseJSON()
+	assert.NilError(t, err)
 
 	for _, v := range hitMap {
 		assert.Equal(t, v, 1)
@@ -62,7 +65,7 @@ func Test_PathMustBeCorrectEveryTime(t *testing.T) {
 	err := json.Unmarshal(document, &originalJSON)
 	assert.NilError(t, err)
 
-	traversal := NewTraversal(originalJSON, OnlyForLeafs(func(data *ActionData) (interface{}, error) {
+	traversal := NewTraversal(originalJSON, OnlyForLeafsAndKeys(func(data *ActionData) (interface{}, error) {
 		if data.Element.(string) == expectedValue {
 			assert.Equal(t, expectedPath, data.Path)
 		}
@@ -70,4 +73,5 @@ func Test_PathMustBeCorrectEveryTime(t *testing.T) {
 	}))
 
 	_, err = traversal.TraverseJSON()
+	assert.NilError(t, err)
 }

pkg/engine/variables/vars.go

@@ -113,7 +113,7 @@ func ValidateBackgroundModeVars(log logr.Logger, ctx context.EvalInterface, rule
 }
 
 func validateBackgroundModeVars(log logr.Logger, ctx context.EvalInterface) jsonUtils.Action {
-	return jsonUtils.OnlyForLeafs(func(data *jsonUtils.ActionData) (interface{}, error) {
+	return jsonUtils.OnlyForLeafsAndKeys(func(data *jsonUtils.ActionData) (interface{}, error) {
 		value, ok := data.Element.(string)
 		if !ok {
 			return data.Element, nil
@@ -149,7 +149,7 @@ func (n NotResolvedReferenceErr) Error() string {
 }
 
 func substituteReferencesIfAny(log logr.Logger) jsonUtils.Action {
-	return jsonUtils.OnlyForLeafs(func(data *jsonUtils.ActionData) (interface{}, error) {
+	return jsonUtils.OnlyForLeafsAndKeys(func(data *jsonUtils.ActionData) (interface{}, error) {
 		value, ok := data.Element.(string)
 		if !ok {
 			return data.Element, nil
@@ -196,7 +196,7 @@ func DefaultVariableResolver(ctx context.EvalInterface, variable string) (interf
 }
 
 func substituteVariablesIfAny(log logr.Logger, ctx context.EvalInterface, vr VariableResolver) jsonUtils.Action {
-	return jsonUtils.OnlyForLeafs(func(data *jsonUtils.ActionData) (interface{}, error) {
+	return jsonUtils.OnlyForLeafsAndKeys(func(data *jsonUtils.ActionData) (interface{}, error) {
 		value, ok := data.Element.(string)
 		if !ok {
 			return data.Element, nil
@@ -359,7 +359,7 @@ func formAbsolutePath(referencePath, absolutePath string) string {
 func getValueFromReference(fullDocument interface{}, path string) (interface{}, error) {
 	var element interface{}
 
-	if _, err := jsonUtils.NewTraversal(fullDocument, jsonUtils.OnlyForLeafs(
+	if _, err := jsonUtils.NewTraversal(fullDocument, jsonUtils.OnlyForLeafsAndKeys(
 		func(data *jsonUtils.ActionData) (interface{}, error) {
 			if common.RemoveAnchorsFromPath(data.Path) == path {
 				element = data.Element

test/e2e/mutate/config.go

@@ -76,6 +76,16 @@ var tests = []struct {
 		ResourceRaw:        podWithContainersAndInitContainers,
 		ExpectedPatternRaw: podWithContainersAndInitContainersPattern,
 	},
+	{
+		TestDescription:    "checks that variables in the keys are working correctly",
+		PolicyName:         "structured-logs-sidecar",
+		PolicyRaw:          kyverno_2316_policy,
+		ResourceName:       "busybox",
+		ResourceNamespace:  "test-mutate2",
+		ResourceGVR:        deploymentGVR,
+		ResourceRaw:        kyverno_2316_resource,
+		ExpectedPatternRaw: kyverno_2316_pattern,
+	},
 }
 
 var ingressTests = struct {

test/e2e/mutate/mutate_test.go

@@ -43,11 +43,11 @@ func Test_Mutate_Sets(t *testing.T) {
 
 		// Clean up Resources
 		By("Cleaning Cluster Policies")
-		_ = e2eClient.CleanClusterPolicies(policyGVR)
+		e2eClient.CleanClusterPolicies(policyGVR)
 
 		// Clear Namespace
 		By(fmt.Sprintf("Deleting Namespace : %s", tests.ResourceNamespace))
-		_ = e2eClient.DeleteClusteredResource(namespaceGVR, tests.ResourceNamespace)
+		e2eClient.DeleteClusteredResource(namespaceGVR, tests.ResourceNamespace)
 
 		// Wait Till Deletion of Namespace
 		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
@@ -96,7 +96,7 @@ func Test_Mutate_Sets(t *testing.T) {
 		// Verify created ConfigMap
 		By(fmt.Sprintf("Verifying ConfigMap in the Namespace : %s", tests.ResourceNamespace))
 		// Wait Till Creation of ConfigMap
-		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
+		e2e.GetWithRetry(1*time.Second, 15, func() error {
 			_, err := e2eClient.GetNamespacedResource(configMapGVR, tests.ResourceNamespace, "target")
 			if err != nil {
 				return err
@@ -113,10 +113,10 @@ func Test_Mutate_Sets(t *testing.T) {
 		Expect(cmRes.GetLabels()["kyverno.key/copy-me"]).To(Equal("sample-value"))
 
 		//CleanUp Resources
-		_ = e2eClient.CleanClusterPolicies(policyGVR)
+		e2eClient.CleanClusterPolicies(policyGVR)
 
 		// Clear Namespace
-		_ = e2eClient.DeleteClusteredResource(namespaceGVR, tests.ResourceNamespace)
+		e2eClient.DeleteClusteredResource(namespaceGVR, tests.ResourceNamespace)
 
 		// Wait Till Deletion of Namespace
 		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
@@ -145,14 +145,14 @@ func Test_Mutate(t *testing.T) {
 		By(fmt.Sprintf("Mutation Test: %s", test.TestDescription))
 
 		By("Deleting Cluster Policies...")
-		_ = e2eClient.CleanClusterPolicies(policyGVR)
+		e2eClient.CleanClusterPolicies(policyGVR)
 
 		By("Deleting Resource...")
-		_ = e2eClient.DeleteNamespacedResource(test.ResourceGVR, test.ResourceNamespace, test.ResourceName)
+		e2eClient.DeleteNamespacedResource(test.ResourceGVR, test.ResourceNamespace, test.ResourceName)
 
 		By("Deleting Namespace...")
 		By(fmt.Sprintf("Deleting Namespace: %s...", test.ResourceNamespace))
-		_ = e2eClient.DeleteClusteredResource(namespaceGVR, test.ResourceNamespace)
+		e2eClient.DeleteClusteredResource(namespaceGVR, test.ResourceNamespace)
 
 		By("Wait Till Deletion of Namespace...")
 		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
@@ -232,7 +232,7 @@ func Test_Mutate(t *testing.T) {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Wait Till Creation of Namespace...")
-		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
+		e2e.GetWithRetry(1*time.Second, 15, func() error {
 			_, err := e2eClient.GetClusteredResource(namespaceGVR, test.ResourceNamespace)
 			if err != nil {
 				return nil
@@ -257,11 +257,11 @@ func Test_Mutate_Ingress(t *testing.T) {
 	Expect(err).To(BeNil())
 
 	nspace := ingressTests.testNamesapce
-	By(fmt.Sprintf("Cleaning Cluster Policies"))
-	_ = e2eClient.CleanClusterPolicies(policyGVR)
+	By("Cleaning Cluster Policies")
+	e2eClient.CleanClusterPolicies(policyGVR)
 
 	By(fmt.Sprintf("Deleting Namespace : %s", nspace))
-	_ = e2eClient.DeleteClusteredResource(namespaceGVR, nspace)
+	e2eClient.DeleteClusteredResource(namespaceGVR, nspace)
 
 	// Wait Till Deletion of Namespace
 	err = e2e.GetWithRetry(1*time.Second, 15, func() error {
@@ -273,7 +273,7 @@ func Test_Mutate_Ingress(t *testing.T) {
 	})
 	Expect(err).To(BeNil())
 
-	By(fmt.Sprintf("Creating mutate ClusterPolicy "))
+	By("Creating mutate ClusterPolicy")
 	_, err = e2eClient.CreateClusteredResourceYaml(policyGVR, ingressTests.cpol)
 	Expect(err).NotTo(HaveOccurred())
 
@@ -303,7 +303,7 @@ func Test_Mutate_Ingress(t *testing.T) {
 		})
 		Expect(err).To(BeNil())
 
-		By(fmt.Sprintf("Comparing patched field"))
+		By("Comparing patched field")
 		rules, ok, err := unstructured.NestedSlice(mutatedResource.UnstructuredContent(), "spec", "rules")
 		Expect(err).To(BeNil())
 		Expect(ok).To(BeTrue())

test/e2e/mutate/resources.go

@@ -7,6 +7,7 @@ import (
 )
 
 var podGVR = e2e.GetGVR("", "v1", "pods")
+var deploymentGVR = e2e.GetGVR("apps", "v1", "deployments")
 
 func newNamespaceYaml(name string) []byte {
 	ns := fmt.Sprintf(`
@@ -307,3 +308,81 @@ spec:
     securityContext:
       runAsNonRoot: true
 `)
+
+var kyverno_2316_policy = []byte(`
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: structured-logs-sidecar
+spec:
+  background: false
+  rules:
+    - name: add-annotations
+      match:
+        resources:
+          kinds:
+            - Deployment
+          annotations:
+            structured-logs: "true"
+      mutate:
+        patchStrategicMerge:
+          metadata:
+            annotations:
+              "fluentbit.io/exclude-{{request.object.spec.template.spec.containers[0].name}}": "true"
+`)
+
+var kyverno_2316_resource = []byte(`
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: busybox
+  namespace: test-mutate2
+  annotations:
+    structured-logs: "true"
+  labels:
+    # app: busybox
+    color: red
+    animal: bear
+    food: pizza
+    car: jeep
+    env: qa
+    # foo: blaaah
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      appa: busybox
+  template:
+    metadata:
+      labels:
+        appa: busybox
+        # foo: blaaah
+    spec:
+      containers:
+      - image: busybox:1.28
+        name: busybox
+        command: ["sleep", "9999"]
+        resources:
+          requests:
+            cpu: 100m
+            memory: 10Mi
+          limits:
+            cpu: 100m
+            memory: 10Mi
+      - image: busybox:1.28
+        name: busybox1
+        command: ["sleep", "9999"]
+        resources:
+          requests:
+            cpu: 100m
+            memory: 10Mi
+          limits:
+            cpu: 100m
+            memory: 20Mi
+`)
+
+var kyverno_2316_pattern = []byte(`
+metadata:
+  annotations:
+    fluentbit.io/exclude-busybox: "true"
+`)