refactor: ResourceDescription validation (#3446)

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

api/kyverno/v1/resource_description_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"gotest.tools/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
@@ -17,6 +18,36 @@ func Test_ResourceDescription(t *testing.T) {
 		name:       "valid",
 		namespaced: true,
 		subject:    ResourceDescription{},
+	}, {
+		name:       "name-names",
+		namespaced: true,
+		subject: ResourceDescription{
+			Name:  "foo",
+			Names: []string{"bar", "baz"},
+		},
+		errors: []string{
+			`dummy: Invalid value: v1.ResourceDescription{Kinds:[]string(nil), Name:"foo", Names:[]string{"bar", "baz"}, Namespaces:[]string(nil), Annotations:map[string]string(nil), Selector:(*v1.LabelSelector)(nil), NamespaceSelector:(*v1.LabelSelector)(nil)}: Both name and names can not be specified together`,
+		},
+	}, {
+		name:       "selector",
+		namespaced: true,
+		subject: ResourceDescription{
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"app.type": "prod",
+				},
+			},
+		},
+	}, {
+		name:       "bad-selector",
+		namespaced: true,
+		subject: ResourceDescription{
+			Kinds:    []string{"Deployment"},
+			Selector: &metav1.LabelSelector{},
+		},
+		errors: []string{
+			`dummy.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string(nil), MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: The requirements are not specified in selector`,
+		},
 	}, {
 		name:       "namespaces",
 		namespaced: true,

api/kyverno/v1/resource_description_types.go

@@ -55,6 +55,19 @@ type ResourceDescription struct {
 // Validate implements programmatic validation
 func (r *ResourceDescription) Validate(path *field.Path, namespaced bool, clusterResources sets.String) field.ErrorList {
 	var errs field.ErrorList
+	if r.Name != "" && len(r.Names) > 0 {
+		errs = append(errs, field.Invalid(path, r, "Both name and names can not be specified together"))
+	}
+	if r.Selector != nil && !labelSelectorContainsWildcard(r.Selector) {
+		if selector, err := metav1.LabelSelectorAsSelector(r.Selector); err != nil {
+			errs = append(errs, field.Invalid(path.Child("selector"), r.Selector, err.Error()))
+		} else {
+			requirements, _ := selector.Requirements()
+			if len(requirements) == 0 {
+				errs = append(errs, field.Invalid(path.Child("selector"), r.Selector, "The requirements are not specified in selector"))
+			}
+		}
+	}
 	if namespaced {
 		if len(r.Namespaces) > 0 {
 			errs = append(errs, field.Forbidden(path.Child("namespaces"), "Filtering namespaces not allowed in namespaced policies"))

api/kyverno/v1/utils.go

@@ -1,8 +1,11 @@
 package v1
 
 import (
+	"strings"
+
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	log "sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -36,6 +39,22 @@ func ValidatePolicyName(path *field.Path, name string) field.ErrorList {
 	return errs
 }
 
+func labelSelectorContainsWildcard(v *metav1.LabelSelector) bool {
+	for k, v := range v.MatchLabels {
+		if isWildcardPresent(k) || isWildcardPresent(v) {
+			return true
+		}
+	}
+	return false
+}
+
+func isWildcardPresent(v string) bool {
+	if strings.Contains(v, "*") || strings.Contains(v, "?") {
+		return true
+	}
+	return false
+}
+
 // ViolatedRule stores the information regarding the rule.
 type ViolatedRule struct {
 	// Name specifies violated rule name.

pkg/policy/validate.go

@@ -916,27 +916,6 @@ func validateResources(path *field.Path, rule kyverno.Rule) (string, error) {
 		}
 	}
 
-	if len(rule.ExcludeResources.Any) > 0 {
-		for _, rmr := range rule.ExcludeResources.Any {
-			// exclude resources
-			if path, err := validateExcludeResourceDescription(rmr.ResourceDescription); err != nil {
-				return fmt.Sprintf("exclude.resources.%s", path), err
-			}
-		}
-	} else if len(rule.ExcludeResources.All) > 0 {
-		for _, rmr := range rule.ExcludeResources.All {
-			// exclude resources
-			if path, err := validateExcludeResourceDescription(rmr.ResourceDescription); err != nil {
-				return fmt.Sprintf("exclude.resources.%s", path), err
-			}
-		}
-	} else {
-		// exclude resources
-		if path, err := validateExcludeResourceDescription(rule.ExcludeResources.ResourceDescription); err != nil {
-			return fmt.Sprintf("exclude.resources.%s", path), err
-		}
-	}
-
 	//validating the values present under validate.preconditions, if they exist
 	if target := rule.GetAnyAllConditions(); target != nil {
 		if path, err := validateConditions(target, "preconditions"); err != nil {
@@ -1216,72 +1195,9 @@ func validateMatchedResourceDescription(rd kyverno.ResourceDescription) (string,
 		return "", fmt.Errorf("match resources not specified")
 	}
 
-	if rd.Name != "" && len(rd.Names) > 0 {
-		return "", fmt.Errorf("both name and names can not be specified together")
-	}
-
-	if err := validateResourceDescription(rd); err != nil {
-		return "match", err
-	}
-
 	return "", nil
 }
 
-func validateExcludeResourceDescription(rd kyverno.ResourceDescription) (string, error) {
-	if reflect.DeepEqual(rd, kyverno.ResourceDescription{}) {
-		// exclude is not mandatory
-		return "", nil
-	}
-
-	if rd.Name != "" && len(rd.Names) > 0 {
-		return "", fmt.Errorf("both name and names can not be specified together")
-	}
-
-	if err := validateResourceDescription(rd); err != nil {
-		return "exclude", err
-	}
-	return "", nil
-}
-
-// validateResourceDescription returns error if selector is invalid
-// field type is checked through openapi
-func validateResourceDescription(rd kyverno.ResourceDescription) error {
-	if rd.Selector != nil {
-		if labelSelectorContainsWildcard(rd.Selector) {
-			return nil
-		}
-
-		selector, err := metav1.LabelSelectorAsSelector(rd.Selector)
-		if err != nil {
-			return err
-		}
-		requirements, _ := selector.Requirements()
-		if len(requirements) == 0 {
-			return errors.New("the requirements are not specified in selector")
-		}
-	}
-	return nil
-}
-
-func labelSelectorContainsWildcard(v *metav1.LabelSelector) bool {
-	for k, v := range v.MatchLabels {
-		if isWildcardPresent(k) {
-			return true
-		}
-		if isWildcardPresent(v) {
-			return true
-		}
-	}
-	return false
-}
-
-func isWildcardPresent(v string) bool {
-	if strings.Contains(v, "*") || strings.Contains(v, "?") {
-		return true
-	}
-	return false
-}
-
 // checkClusterResourceInMatchAndExclude returns false if namespaced ClusterPolicy contains cluster wide resources in
 // Match and Exclude block
 func checkClusterResourceInMatchAndExclude(rule kyverno.Rule, clusterResources sets.String, mock bool, res []*metav1.APIResourceList) error {

pkg/policy/validate_test.go

@@ -261,44 +261,6 @@ func Test_Validate_PreconditionsValuesList_KeyRequestOperation_UnknownItem(t *te
 	assert.Assert(t, err != nil)
 }
 
-func Test_Validate_ResourceDescription_MissingKindsOnExclude(t *testing.T) {
-	var err error
-	excludeResourcedescirption := []byte(`
-	{
-		"selector": {
-		   "matchLabels": {
-			  "app.type": "prod"
-		   }
-		}
-	 }`)
-
-	var rd kyverno.ResourceDescription
-	err = json.Unmarshal(excludeResourcedescirption, &rd)
-	assert.NilError(t, err)
-
-	_, err = validateExcludeResourceDescription(rd)
-	assert.NilError(t, err)
-}
-
-func Test_Validate_ResourceDescription_InvalidSelector(t *testing.T) {
-	rawResourcedescirption := []byte(`
-	{
-		"kinds": [
-		   "Deployment"
-		],
-		"selector": {
-		   "app.type": "prod"
-		}
-	 }`)
-
-	var rd kyverno.ResourceDescription
-	err := json.Unmarshal(rawResourcedescirption, &rd)
-	assert.NilError(t, err)
-
-	err = validateResourceDescription(rd)
-	assert.Assert(t, err != nil)
-}
-
 func Test_Validate_Policy(t *testing.T) {
 	rawPolicy := []byte(`
 	{