Repository: kyverno/kyverno
Pull request #1800 (2021-04-13 21:21:29)

Fix array variables substitution
Signed-off-by: Shuting Zhao <shutting06@gmail.com>

## Related issue

Fixes https://github.com/kyverno/kyverno/issues/1777.

ConditionOperator `In` and `NotIn` operates on a set of values. The variable substation for these types of values is not handled properly in the previous PR.

<!--
Please link the GitHub issue this pull request resolves in the format of `#1234`. If you discussed this change
with a maintainer, please mention her/him using the `@` syntax (e.g. `@JimBugwadia`).

If this change neither resolves an existing issue nor has sign-off from one of the maintainers, there is a
chance substantial changes will be requested or that the changes will be rejected.

You can discuss changes with maintainers in the [Kyverno Slack Channel](https://kubernetes.slack.com/).
-->

## What type of PR is this
> /kind bug
<!--

> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading white spaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
-->

## Proposed Changes



<!--
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. 

***NOTE***: If this PR results in new or altered behavior which is user facing, you **MUST** read and follow the steps outlined in the [PR documentation guide](pr_documentation.md) and add Proof Manifests as defined below.
-->

### Proof Manifests
1. Follow the reproduce steps described in https://github.com/kyverno/kyverno/issues/1777, this is the output:
```
Error from server: error when creating "resource.yaml": admission webhook "validate.kyverno.svc" denied the request:

resource Deployment/default/busybox was blocked due to the following policies

cm-array-example:
  validate-role-annotation: 'The animal cow is not in the allowed list of animals: ["snake","bear","cat","dog"].'
```
2. If I change `validate.deny.operator` to `In`, the resource creation is allowed:
```
 ✗ k apply -f resource.yaml
deployment.apps/busybox created
```
 
<!--
Read and follow the [PR documentation guide](pr_documentation.md) for more details first. This section is for pasting your YAML manifests (Kubernetes resources and Kyverno policies) which allow maintainers to prove the intended functionality is achieved by your PR. Please use proper fenced code block formatting, for example:

```yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: roles-dictionary
  namespace: default
data:
  allowed-roles: "[\"cluster-admin\", \"cluster-operator\", \"tenant-admin\"]"
```
-->

## Checklist

<!--
Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of
them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
-->

- [x] I have read the [contributing guidelines](https://github.com/kyverno/kyverno/blob/main/CONTRIBUTING.md).
- [x] I have added tests that prove my fix is effective or that my feature works.
- [] My PR contains new or altered behavior to Kyverno and
  - [] I have added or changed [the documentation](https://github.com/kyverno/website) myself in an existing PR and the link is:
  <!-- Uncomment to link to the PR -->
  <!-- https://github.com/kyverno/website/pull/123 -->
  - [] I have raised an issue in [kyverno/website](https://github.com/kyverno/website) to track the doc update and the link is:
  <!-- Uncomment to link to the issue -->
  <!-- https://github.com/kyverno/website/issues/1 -->
  - [] I have read the [PR documentation guide](pr_documentation.md) and followed the process including adding proof manifests to this PR.

## Further Comments

<!--
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc...
-->

cc @kacejot 


Patch:
diff --git a/pkg/engine/variables/vars.go b/pkg/engine/variables/vars.go
index 1173d314d..c8a2d005b 100644
--- a/pkg/engine/variables/vars.go
+++ b/pkg/engine/variables/vars.go
@@ -104,7 +104,7 @@ func validateBackgroundModeVars(log logr.Logger, ctx context.EvalInterface) json
 				case context.InvalidVariableErr:
 					return nil, err
 				default:
-					return nil, fmt.Errorf("failed to resolve %v at path %s", variable, data.Path)
+					return nil, fmt.Errorf("failed to resolve %v at path %s: %v", variable, data.Path, err)
 				}
 			}
 		}
@@ -146,12 +146,12 @@ func substituteReferencesIfAny(log logr.Logger) jsonUtils.Action {
 				case context.InvalidVariableErr:
 					return nil, err
 				default:
-					return nil, fmt.Errorf("failed to resolve %v at path %s", v, data.Path)
+					return nil, fmt.Errorf("failed to resolve %v at path %s: %v", v, data.Path, err)
 				}
 			}
 
 			if resolvedReference == nil {
-				return data.Element, fmt.Errorf("failed to resolve %v at path %s", v, data.Path)
+				return data.Element, fmt.Errorf("failed to resolve %v at path %s: %v", v, data.Path, err)
 			}
 
 			log.V(3).Info("reference resolved", "reference", v, "value", resolvedReference, "path", data.Path)
@@ -178,9 +178,10 @@ func substituteVariablesIfAny(log logr.Logger, ctx context.EvalInterface) jsonUt
 			return data.Element, nil
 		}
 
-		originalPattern := value
 		vars := regexVariables.FindAllString(value, -1)
 		for len(vars) > 0 {
+			originalPattern := value
+
 			for _, v := range vars {
 				variable := replaceBracesAndTrimSpaces(v)
 
@@ -195,23 +196,22 @@ func substituteVariablesIfAny(log logr.Logger, ctx context.EvalInterface) jsonUt
 					case context.InvalidVariableErr:
 						return nil, err
 					default:
-						return nil, fmt.Errorf("failed to resolve %v at path %s", variable, data.Path)
+						return nil, fmt.Errorf("failed to resolve %v at path %s: %v", variable, data.Path, err)
 					}
 				}
 
 				log.V(3).Info("variable substituted", "variable", v, "value", substitutedVar, "path", data.Path)
 
-				if val, ok := substitutedVar.(string); ok {
-					value = strings.Replace(value, v, val, -1)
-					continue
-				}
-
 				if substitutedVar != nil {
 					if originalPattern == v {
 						return substitutedVar, nil
 					}
 
-					return nil, fmt.Errorf("failed to resolve %v at path %s", variable, data.Path)
+					if value, err = substituteVarInPattern(originalPattern, v, substitutedVar); err != nil {
+						return nil, fmt.Errorf("failed to resolve %v at path %s: %s", variable, data.Path, err.Error())
+					}
+
+					continue
 				}
 
 				return nil, NotFoundVariableErr{
@@ -228,6 +228,22 @@ func substituteVariablesIfAny(log logr.Logger, ctx context.EvalInterface) jsonUt
 	})
 }
 
+func substituteVarInPattern(pattern, variable string, value interface{}) (string, error) {
+	var stringToSubstitute string
+
+	if s, ok := value.(string); ok {
+		stringToSubstitute = s
+	} else {
+		buffer, err := json.Marshal(value)
+		if err != nil {
+			return "", fmt.Errorf("failed to marshal %T: %v", value, value)
+		}
+		stringToSubstitute = string(buffer)
+	}
+
+	return strings.Replace(pattern, variable, stringToSubstitute, -1), nil
+}
+
 func replaceBracesAndTrimSpaces(v string) string {
 	variable := strings.ReplaceAll(v, "{{", "")
 	variable = strings.ReplaceAll(variable, "}}", "")
diff --git a/pkg/policyreport/builder.go b/pkg/policyreport/builder.go
index 8d602305a..1cb29ceab 100755
--- a/pkg/policyreport/builder.go
+++ b/pkg/policyreport/builder.go
@@ -132,7 +132,7 @@ func (builder *requestBuilder) build(info Info) (req *unstructured.Unstructured,
 }
 
 func (builder *requestBuilder) buildRCRResult(policy string, resource response.ResourceSpec, rule kyverno.ViolatedRule) *report.PolicyReportResult {
-	av := builder.fetchAnnoationValues(policy, resource.Namespace)
+	av := builder.fetchAnnotationValues(policy, resource.Namespace)
 
 	result := &report.PolicyReportResult{
 		Policy: policy,
@@ -275,9 +275,9 @@ func (av *annotationValues) setSeverityFromString(severity string) {
 	}
 }
 
-func (builder *requestBuilder) fetchAnnoationValues(policy, ns string) annotationValues {
+func (builder *requestBuilder) fetchAnnotationValues(policy, ns string) annotationValues {
 	av := annotationValues{}
-	ann := builder.fetchAnnoations(policy, ns)
+	ann := builder.fetchAnnotations(policy, ns)
 
 	if category, ok := ann[categoryLabel]; ok {
 		av.category = category
@@ -289,7 +289,7 @@ func (builder *requestBuilder) fetchAnnoationValues(policy, ns string) annotatio
 	return av
 }
 
-func (builder *requestBuilder) fetchAnnoations(policy, ns string) map[string]string {
+func (builder *requestBuilder) fetchAnnotations(policy, ns string) map[string]string {
 	cpol, err := builder.cpolLister.Get(policy)
 	if err == nil {
 		if ann := cpol.GetAnnotations(); ann != nil {


Test patch:
diff --git a/pkg/engine/validation_test.go b/pkg/engine/validation_test.go
index 33cc252c5..1b09f3aa8 100644
--- a/pkg/engine/validation_test.go
+++ b/pkg/engine/validation_test.go
@@ -1415,6 +1415,65 @@ func Test_VariableSubstitutionPathNotExistInAnyPattern_OnePatternStatisfiesButSu
 	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "variable substitution failed for rule test-path-not-exist: variable request.object.metadata.name1 not resolved at path /validate/anyPattern/0/spec/template/spec/containers/0/name")
 }
 
+func Test_VariableSubstitution_NotOperatorWithStringVariable(t *testing.T) {
+	resourceRaw := []byte(`{
+		"apiVersion": "v1",
+		"kind": "Deployment",
+		"metadata": {
+		  "name": "test"
+		},
+		"spec": {
+		  "content": "sample text"
+		}
+	  }`)
+
+	policyraw := []byte(`{
+		"apiVersion": "kyverno.io/v1",
+		"kind": "ClusterPolicy",
+		"metadata": {
+		  "name": "substitute-variable"
+		},
+		"spec": {
+		  "rules": [
+			{
+			  "name": "not-operator-with-variable-should-alway-fail-validation",
+			  "match": {
+				"resources": {
+				  "kinds": [
+					"Deployment"
+				  ]
+				}
+			  },
+			  "validate": {
+				"pattern": {
+			      "spec": {
+				    "content": "!{{ request.object.spec.content }}"
+				  }
+				}
+			  }
+			}
+		  ]
+		}
+	  }`)
+
+	var policy kyverno.ClusterPolicy
+	assert.NilError(t, json.Unmarshal(policyraw, &policy))
+	resourceUnstructured, err := utils.ConvertToUnstructured(resourceRaw)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	err = ctx.AddResource(resourceRaw)
+	assert.NilError(t, err)
+
+	policyContext := &PolicyContext{
+		Policy:      policy,
+		JSONContext: ctx,
+		NewResource: *resourceUnstructured}
+	er := Validate(policyContext)
+	assert.Assert(t, !er.PolicyResponse.Rules[0].Success)
+	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "validation error: rule not-operator-with-variable-should-alway-fail-validation failed at path /spec/content/")
+}
+
 func Test_VariableSubstitutionPathNotExistInAnyPattern_AllPathNotPresent(t *testing.T) {
 	resourceRaw := []byte(`{
 		"apiVersion": "v1",
@@ -1669,7 +1728,12 @@ func Test_VariableSubstitutionValidate_VariablesInMessageAreResolved(t *testing.
 					{
 					  "key": "{{ request.object.metadata.labels.animal }}",
 					  "operator": "NotIn",
-					  "value": "abcde"
+					  "value": [
+						"snake",
+						"bear",
+						"cat",
+						"dog"
+					]
 					}
 				  ]
 				}
@@ -1693,7 +1757,7 @@ func Test_VariableSubstitutionValidate_VariablesInMessageAreResolved(t *testing.
 		JSONContext: ctx,
 		NewResource: *resourceUnstructured}
 	er := Validate(policyContext)
-	assert.Assert(t, er.PolicyResponse.Rules[0].Success)
+	assert.Assert(t, !er.PolicyResponse.Rules[0].Success)
 	assert.Equal(t, er.PolicyResponse.Rules[0].Message, "The animal cow is not in the allowed list of animals.")
 }
 
diff --git a/pkg/engine/variables/variables_test.go b/pkg/engine/variables/variables_test.go
index a42a8ea06..73771bd65 100644
--- a/pkg/engine/variables/variables_test.go
+++ b/pkg/engine/variables/variables_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	kyverno "github.com/kyverno/kyverno/pkg/api/kyverno/v1"
+	"gotest.tools/assert"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -562,10 +563,26 @@ func Test_variableSubstitutionObjectOperatorNotEqualFail(t *testing.T) {
 		t.Error(err)
 	}
 
-	if patternCopy, err = SubstituteAll(log.Log, ctx, patternCopy); err == nil {
-		t.Error(err)
-	}
+	patternCopy, err = SubstituteAll(log.Log, ctx, patternCopy)
+	assert.NilError(t, err)
+
+	patternMapCopy, ok := patternCopy.(map[string]interface{})
+	assert.Assert(t, ok)
+
+	specInterface, ok := patternMapCopy["spec"]
+	assert.Assert(t, ok)
+
+	specMap, ok := specInterface.(map[string]interface{})
+	assert.Assert(t, ok)
+
+	variableInterface, ok := specMap["variable"]
+	assert.Assert(t, ok)
+
+	variableString, ok := variableInterface.(string)
+	assert.Assert(t, ok)
 
+	expected := `!{"var1":"temp1","var2":"temp2","varNested":{"var1":"temp1"}}`
+	assert.Equal(t, expected, variableString)
 }
 
 func Test_variableSubstitutionMultipleObject(t *testing.T) {
diff --git a/pkg/engine/variables/vars_test.go b/pkg/engine/variables/vars_test.go
index efb5e1a26..6b17f98be 100644
--- a/pkg/engine/variables/vars_test.go
+++ b/pkg/engine/variables/vars_test.go
@@ -6,6 +6,7 @@ import (
 	"strings"
 	"testing"
 
+	v1 "github.com/kyverno/kyverno/pkg/api/kyverno/v1"
 	"github.com/kyverno/kyverno/pkg/engine/context"
 	ju "github.com/kyverno/kyverno/pkg/engine/json-utils"
 	"gotest.tools/assert"
@@ -334,6 +335,341 @@ func Test_policyContextValidation(t *testing.T) {
 	assert.Assert(t, err != nil, err)
 }
 
+func Test_variableSubstitution_array(t *testing.T) {
+	configmapRaw := []byte(`
+{
+    "animals": {
+        "apiVersion": "v1",
+        "kind": "ConfigMap",
+        "metadata": {
+            "name": "animals",
+            "namespace": "default"
+        },
+        "data": {
+            "animals": "snake\nbear\ncat\ndog"
+        }
+    }
+}`)
+
+	ruleRaw := []byte(`
+{
+    "name": "validate-role-annotation",
+    "context": [
+        {
+            "name": "animals",
+            "configMap": {
+                "name": "animals",
+                "namespace": "default"
+            }
+        }
+    ],
+    "match": {
+        "resources": {
+            "kinds": [
+                "Deployment"
+            ]
+        }
+    },
+    "validate": {
+        "message": "The animal {{ request.object.metadata.labels.animal }} is not in the allowed list of animals: {{ animals.data.animals }}.",
+        "deny": {
+            "conditions": [
+                {
+                    "key": "{{ request.object.metadata.labels.animal }}",
+                    "operator": "NotIn",
+                    "value": "{{ animals.data.animals }}"
+                }
+            ]
+        }
+    }
+}`)
+
+	resourceRaw := []byte(`
+{
+    "apiVersion": "apps/v1",
+    "kind": "Deployment",
+    "metadata": {
+        "name": "busybox",
+        "labels": {
+            "app": "busybox",
+            "color": "red",
+            "animal": "cow",
+            "food": "pizza",
+            "car": "jeep",
+            "env": "qa"
+        }
+    }
+}
+`)
+
+	var rule v1.Rule
+	err := json.Unmarshal(ruleRaw, &rule)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext("request.object", "animals")
+	ctx.AddJSON(configmapRaw)
+	ctx.AddResource(resourceRaw)
+
+	vars, err := SubstituteAllInRule(log.Log, ctx, rule)
+	assert.NilError(t, err)
+
+	assert.DeepEqual(t, vars.Validation.Message, "The animal cow is not in the allowed list of animals: snake\nbear\ncat\ndog.")
+}
+
+var variableObject = []byte(`
+{
+	"complex_object_array": [
+		"value1",
+		"value2",
+		"value3"
+	],
+	"complex_object_map": {
+		"key1": "value1",
+		"key2": "value2",
+		"key3": "value3"
+	},
+	"simple_object_bool": false,
+	"simple_object_int": 5,
+	"simple_object_float": -5.5,
+	"simple_object_string": "example",
+	"simple_object_null": null
+}
+`)
+
+func Test_SubstituteArray(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "{{ request.object.complex_object_array }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := resource["complex_object_array"]
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteArrayInString(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "content is {{ request.object.complex_object_map }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := `content is {"key1":"value1","key2":"value2","key3":"value3"}`
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteInt(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "{{ request.object.simple_object_int }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := resource["simple_object_int"]
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteIntInString(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "content = {{ request.object.simple_object_int }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := "content = 5"
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteBool(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "{{ request.object.simple_object_bool }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := false
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteBoolInString(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "content = {{ request.object.simple_object_bool }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	expected := "content = false"
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteString(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "{{ request.object.simple_object_string }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	var expected interface{}
+	expected = "example"
+
+	assert.DeepEqual(t, expected, content)
+}
+
+func Test_SubstituteStringInString(t *testing.T) {
+	patternRaw := []byte(`
+	{
+		"spec": {
+			"content": "content = {{ request.object.simple_object_string }}"
+		}
+	}
+	`)
+
+	var err error
+	var pattern, resource map[string]interface{}
+	err = json.Unmarshal(patternRaw, &pattern)
+	assert.NilError(t, err)
+
+	err = json.Unmarshal(variableObject, &resource)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext()
+	ctx.AddResource(variableObject)
+
+	resolved, err := SubstituteAll(log.Log, ctx, pattern)
+	assert.NilError(t, err)
+
+	content := resolved.(map[string]interface{})["spec"].(map[string]interface{})["content"]
+	var expected interface{}
+	expected = "content = example"
+
+	assert.DeepEqual(t, expected, content)
+}
+
 func Test_ReferenceSubstitution(t *testing.T) {
 	jsonRaw := []byte(`
 	{