From 51254b2d5a27cf8ba858cbf08865374c9227dfab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?=
<charled.breteche@gmail.com>
Date: Tue, 22 Mar 2022 14:17:51 +0100
Subject: [PATCH] refactor: ResourceDescription validation (#3446)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>
---
api/kyverno/v1/resource_description_test.go | 31 ++++++++
api/kyverno/v1/resource_description_types.go | 13 +++
api/kyverno/v1/utils.go | 19 +++++
pkg/policy/validate.go | 84 --------------------
pkg/policy/validate_test.go | 38 ---------
5 files changed, 63 insertions(+), 122 deletions(-)
diff --git a/api/kyverno/v1/resource_description_test.go b/api/kyverno/v1/resource_description_test.go
index 4875681028..41e7ce9e72 100644
--- a/api/kyverno/v1/resource_description_test.go
+++ b/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,
diff --git a/api/kyverno/v1/resource_description_types.go b/api/kyverno/v1/resource_description_types.go
index 0752d38004..3f8d66bac7 100644
--- a/api/kyverno/v1/resource_description_types.go
+++ b/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"))
diff --git a/api/kyverno/v1/utils.go b/api/kyverno/v1/utils.go
index 76dcb60677..f86488eb3d 100755
--- a/api/kyverno/v1/utils.go
+++ b/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.
diff --git a/pkg/policy/validate.go b/pkg/policy/validate.go
index 063f1c1cb6..e358f85260 100644
--- a/pkg/policy/validate.go
+++ b/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 {
diff --git a/pkg/policy/validate_test.go b/pkg/policy/validate_test.go
index 286eb21928..dae2f53524 100644
--- a/pkg/policy/validate_test.go
+++ b/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(`
{