Repository: kyverno/kyverno
Pull request #1646 (2021-02-26 07:50:19)

fix: removed validator
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>

## Related issue
Fix for bug in #1641 

<!--
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 cleanup

<!--

> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /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.
-->

## 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).
- [] I have added tests that prove my fix is effective or that my feature works.
- [] I have added or changed [the documentation](https://github.com/kyverno/website).

## 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...
-->



Patch:
diff --git a/charts/kyverno/crds/crds.yaml b/charts/kyverno/crds/crds.yaml
index 12a616d8f..a0391c3b2 100644
--- a/charts/kyverno/crds/crds.yaml
+++ b/charts/kyverno/crds/crds.yaml
@@ -400,7 +400,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -431,7 +430,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
@@ -1582,7 +1580,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -1613,7 +1610,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
diff --git a/definitions/crds/kyverno.io_clusterpolicies.yaml b/definitions/crds/kyverno.io_clusterpolicies.yaml
index 2b1cdfd36..19e1a413d 100644
--- a/definitions/crds/kyverno.io_clusterpolicies.yaml
+++ b/definitions/crds/kyverno.io_clusterpolicies.yaml
@@ -604,7 +604,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of
                               values. The values can be fixed set or can be variables
@@ -644,7 +643,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or
                                       set of values. The values can be fixed set or
diff --git a/definitions/crds/kyverno.io_policies.yaml b/definitions/crds/kyverno.io_policies.yaml
index 0ea34db6a..ba467a1bf 100644
--- a/definitions/crds/kyverno.io_policies.yaml
+++ b/definitions/crds/kyverno.io_policies.yaml
@@ -605,7 +605,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of
                               values. The values can be fixed set or can be variables
@@ -645,7 +644,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or
                                       set of values. The values can be fixed set or
diff --git a/definitions/install.yaml b/definitions/install.yaml
index f7998542d..a24253229 100644
--- a/definitions/install.yaml
+++ b/definitions/install.yaml
@@ -405,7 +405,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -436,7 +435,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
@@ -1587,7 +1585,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -1618,7 +1615,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
diff --git a/definitions/install_debug.yaml b/definitions/install_debug.yaml
index dee1ce58e..526ea9a2e 100755
--- a/definitions/install_debug.yaml
+++ b/definitions/install_debug.yaml
@@ -405,7 +405,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -436,7 +435,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
@@ -1587,7 +1585,6 @@ spec:
                             - In
                             - NotIn
                             type: string
-                            x-kubernetes-preserve-unknown-fields: true
                           value:
                             description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                             x-kubernetes-preserve-unknown-fields: true
@@ -1618,7 +1615,6 @@ spec:
                                     - In
                                     - NotIn
                                     type: string
-                                    x-kubernetes-preserve-unknown-fields: true
                                   value:
                                     description: Value is the conditional value, or set of values. The values can be fixed set or can be variables declared using using JMESPath.
                                     x-kubernetes-preserve-unknown-fields: true
diff --git a/pkg/api/kyverno/v1/policy_types.go b/pkg/api/kyverno/v1/policy_types.go
index e4d03de00..ef85b121c 100755
--- a/pkg/api/kyverno/v1/policy_types.go
+++ b/pkg/api/kyverno/v1/policy_types.go
@@ -152,7 +152,6 @@ type Condition struct {
 
 	// Operator is the operation to perform. Valid operators
 	// are Equals, NotEquals, In and NotIn.
-	// +kubebuilder:validation:XPreserveUnknownFields
 	Operator ConditionOperator `json:"operator,omitempty" yaml:"operator,omitempty"`
 
 	// Value is the conditional value, or set of values. The values can be fixed set
diff --git a/pkg/engine/variables/operator/in.go b/pkg/engine/variables/operator/in.go
index c02435a76..055abd36d 100644
--- a/pkg/engine/variables/operator/in.go
+++ b/pkg/engine/variables/operator/in.go
@@ -43,6 +43,12 @@ func (in InHandler) Evaluate(key, value interface{}) bool {
 	switch typedKey := key.(type) {
 	case string:
 		return in.validateValueWithStringPattern(typedKey, value)
+	case []interface{}:
+		var stringSlice []string
+		for _, v := range typedKey {
+			stringSlice = append(stringSlice, v.(string))
+		}
+		return in.validateValueWithStringSetPattern(stringSlice, value)
 	default:
 		in.log.Info("Unsupported type", "value", typedKey, "type", fmt.Sprintf("%T", typedKey))
 		return false
@@ -104,7 +110,7 @@ func keyExistsInArray(key string, value interface{}, log logr.Logger) (invalidTy
 }
 
 func (in InHandler) validateValueWithStringSetPattern(key []string, value interface{}) (keyExists bool) {
-	invalidType, keyExists := setExistsInArray(key, value, in.log, false)
+	invalidType, keyExists := setExistsInArray(key, value, in.log)
 	if invalidType {
 		in.log.Info("expected type []string", "value", value, "type", fmt.Sprintf("%T", value))
 		return false
@@ -116,8 +122,7 @@ func (in InHandler) validateValueWithStringSetPattern(key []string, value interf
 // setExistsInArray checks if the key is a subset of value
 // The value can be a string, an array of strings, or a JSON format
 // array of strings (e.g. ["val1", "val2", "val3"].
-// notIn argument is set to true when we want to check if key is NOT a subset of value
-func setExistsInArray(key []string, value interface{}, log logr.Logger, notIn bool) (invalidType bool, keyExists bool) {
+func setExistsInArray(key []string, value interface{}, log logr.Logger) (invalidType bool, keyExists bool) {
 	switch valuesAvailable := value.(type) {
 
 	case []interface{}:
@@ -129,10 +134,8 @@ func setExistsInArray(key []string, value interface{}, log logr.Logger, notIn bo
 			}
 			valueSlice = append(valueSlice, v)
 		}
-		if notIn {
-			return false, checkInSubsetForNotIn(key, valueSlice)
-		}
-		return false, checkInSubset(key, valueSlice)
+
+		return false, isSubset(key, valueSlice)
 
 	case string:
 
@@ -145,18 +148,16 @@ func setExistsInArray(key []string, value interface{}, log logr.Logger, notIn bo
 			log.Error(err, "failed to unmarshal value to JSON string array", "key", key, "value", value)
 			return true, false
 		}
-		if notIn {
-			return false, checkInSubsetForNotIn(key, arr)
-		}
-		return false, checkInSubset(key, arr)
+
+		return false, isSubset(key, arr)
 
 	default:
 		return true, false
 	}
 }
 
-// checkInSubset checks if ALL values of S1 are in S2
-func checkInSubset(key []string, value []string) bool {
+// isSubset checks if S1 is a subset of S2 i.e. ALL values of S1 are in S2
+func isSubset(key []string, value []string) bool {
 	set := make(map[string]int)
 
 	for _, val := range value {
diff --git a/pkg/engine/variables/operator/notin.go b/pkg/engine/variables/operator/notin.go
index bd24a043d..e75b89e86 100644
--- a/pkg/engine/variables/operator/notin.go
+++ b/pkg/engine/variables/operator/notin.go
@@ -41,6 +41,12 @@ func (nin NotInHandler) Evaluate(key, value interface{}) bool {
 	switch typedKey := key.(type) {
 	case string:
 		return nin.validateValueWithStringPattern(typedKey, value)
+	case []interface{}:
+		var stringSlice []string
+		for _, v := range typedKey {
+			stringSlice = append(stringSlice, v.(string))
+		}
+		return nin.validateValueWithStringSetPattern(stringSlice, value)
 	default:
 		nin.log.Info("Unsupported type", "value", typedKey, "type", fmt.Sprintf("%T", typedKey))
 		return false
@@ -58,7 +64,7 @@ func (nin NotInHandler) validateValueWithStringPattern(key string, value interfa
 }
 
 func (nin NotInHandler) validateValueWithStringSetPattern(key []string, value interface{}) bool {
-	invalidType, keyExists := setExistsInArray(key, value, nin.log, true)
+	invalidType, keyExists := setExistsInArray(key, value, nin.log)
 	if invalidType {
 		nin.log.Info("expected type []string", "value", value, "type", fmt.Sprintf("%T", value))
 		return false
@@ -67,24 +73,6 @@ func (nin NotInHandler) validateValueWithStringSetPattern(key []string, value in
 	return !keyExists
 }
 
-// checkInSubsetForNotIn checks if ANY of the values of S1 is in S2
-func checkInSubsetForNotIn(key []string, value []string) bool {
-	set := make(map[string]int)
-
-	for _, val := range value {
-		set[val]++
-	}
-
-	for _, val := range key {
-		_, found := set[val]
-		if found {
-			return true
-		}
-	}
-
-	return false
-}
-
 func (nin NotInHandler) validateValueWithBoolPattern(_ bool, _ interface{}) bool {
 	return false
 }


Test patch:
diff --git a/pkg/engine/variables/evaluate_test.go b/pkg/engine/variables/evaluate_test.go
index 8b1322444..bbdcace2a 100644
--- a/pkg/engine/variables/evaluate_test.go
+++ b/pkg/engine/variables/evaluate_test.go
@@ -1013,3 +1013,105 @@ func Test_Eval_Equal_Var_Fail(t *testing.T) {
 		t.Error("expected to fail")
 	}
 }
+
+// subset test
+
+// test passes if ALL values in "key" are in "value" ("key" is a subset of "value")
+func Test_Eval_In_String_Set_Pass(t *testing.T) {
+	ctx := context.NewContext()
+	key := [2]string{"1.1.1.1", "2.2.2.2"}
+	keyInterface := make([]interface{}, len(key), len(key))
+	for i := range key {
+		keyInterface[i] = key[i]
+	}
+	value := [3]string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}
+	valueInterface := make([]interface{}, len(value), len(value))
+	for i := range value {
+		valueInterface[i] = value[i]
+	}
+
+	condition := kyverno.Condition{
+		Key:      keyInterface,
+		Operator: kyverno.In,
+		Value:    valueInterface,
+	}
+
+	if !Evaluate(log.Log, ctx, condition) {
+		t.Error("expected to pass")
+	}
+}
+
+// test passes if NOT ALL values in "key" are in "value" ("key" is not a subset of "value")
+func Test_Eval_In_String_Set_Fail(t *testing.T) {
+	ctx := context.NewContext()
+	key := [2]string{"1.1.1.1", "4.4.4.4"}
+	keyInterface := make([]interface{}, len(key), len(key))
+	for i := range key {
+		keyInterface[i] = key[i]
+	}
+	value := [3]string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}
+	valueInterface := make([]interface{}, len(value), len(value))
+	for i := range value {
+		valueInterface[i] = value[i]
+	}
+
+	condition := kyverno.Condition{
+		Key:      keyInterface,
+		Operator: kyverno.In,
+		Value:    valueInterface,
+	}
+
+	if Evaluate(log.Log, ctx, condition) {
+		t.Error("expected to fail")
+	}
+}
+
+// test passes if ALL of the values in "key" are NOT in "value" ("key" is not a subset of "value")
+func Test_Eval_NotIn_String_Set_Pass(t *testing.T) {
+	ctx := context.NewContext()
+	key := [2]string{"1.1.1.1", "4.4.4.4"}
+	keyInterface := make([]interface{}, len(key), len(key))
+	for i := range key {
+		keyInterface[i] = key[i]
+	}
+	value := [3]string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}
+	valueInterface := make([]interface{}, len(value), len(value))
+	for i := range value {
+		valueInterface[i] = value[i]
+	}
+
+	condition := kyverno.Condition{
+		Key:      keyInterface,
+		Operator: kyverno.NotIn,
+		Value:    valueInterface,
+	}
+
+	if !Evaluate(log.Log, ctx, condition) {
+		t.Error("expected to pass")
+	}
+}
+
+// test passes if ALL of the values in "key" are in "value" ("key" is a subset of "value")
+func Test_Eval_NotIn_String_Set_Fail(t *testing.T) {
+	ctx := context.NewContext()
+	key := [2]string{"1.1.1.1", "2.2.2.2"}
+	keyInterface := make([]interface{}, len(key), len(key))
+	for i := range key {
+		keyInterface[i] = key[i]
+	}
+	value := [3]string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}
+	valueInterface := make([]interface{}, len(value), len(value))
+	for i := range value {
+		valueInterface[i] = value[i]
+	}
+
+	condition := kyverno.Condition{
+		Key:      keyInterface,
+		Operator: kyverno.NotIn,
+		Value:    valueInterface,
+	}
+
+	if Evaluate(log.Log, ctx, condition) {
+		t.Error("expected to fail")
+	}
+}