From d1d40da8634bfb3f631173f866230e05b1c04ab3 Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes <145056127+britaniar@users.noreply.github.com> Date: Fri, 19 Apr 2024 00:00:45 -0700 Subject: [PATCH] feat: update validation webhook for overrides (#752) --- .../validator/clusterresourceoverride.go | 1 - .../validator/clusterresourceoverride_test.go | 137 ++++- pkg/utils/validator/resourceoverride.go | 94 +++- pkg/utils/validator/resourceoverride_test.go | 493 ++++++++++++++---- 4 files changed, 588 insertions(+), 137 deletions(-) diff --git a/pkg/utils/validator/clusterresourceoverride.go b/pkg/utils/validator/clusterresourceoverride.go index 8b7463eec..41ab0ca42 100644 --- a/pkg/utils/validator/clusterresourceoverride.go +++ b/pkg/utils/validator/clusterresourceoverride.go @@ -30,7 +30,6 @@ func ValidateClusterResourceOverride(cro fleetv1alpha1.ClusterResourceOverride, allErr = append(allErr, err) } - // Check if override rule is using label selector if cro.Spec.Policy != nil { if err := validateOverridePolicy(cro.Spec.Policy); err != nil { allErr = append(allErr, err) diff --git a/pkg/utils/validator/clusterresourceoverride_test.go b/pkg/utils/validator/clusterresourceoverride_test.go index 85d1e91f3..088c11424 100644 --- a/pkg/utils/validator/clusterresourceoverride_test.go +++ b/pkg/utils/validator/clusterresourceoverride_test.go @@ -6,8 +6,9 @@ import ( "strings" "testing" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - apiErrors "k8s.io/apimachinery/pkg/util/errors" + apierrors "k8s.io/apimachinery/pkg/util/errors" fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" @@ -124,7 +125,7 @@ func TestValidateClusterResourceSelectors(t *testing.T) { }, }, }, - wantErrMsg: apiErrors.NewAggregate([]error{fmt.Errorf("label selector is not supported for resource selection %+v", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "Kind", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"key": "value"}}}), + wantErrMsg: apierrors.NewAggregate([]error{fmt.Errorf("label selector is not supported for resource selection %+v", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "Kind", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"key": "value"}}}), fmt.Errorf("resource name is required for resource selection %+v", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "Kind", Name: ""}), fmt.Errorf("resource selector %+v already exists, and must be unique", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "Kind", Name: "example"})}), }, @@ -267,31 +268,42 @@ func TestValidateClusterResourceOverrideResourceLimit(t *testing.T) { } func TestValidateClusterResourceOverride(t *testing.T) { - validPolicy := &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ + validClusterSelector := &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key": "value", - }, - }, - }, - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key1": "value1", - }, - }, - }, + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, + }, + }, + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key1": "value1", }, }, }, }, } + validJSONPatchOverrides := []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, + Path: "/metadata/labels/new-label", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + } + + validPolicy := &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: validClusterSelector, + JSONPatchOverrides: validJSONPatchOverrides, + }, + }, + } + tests := map[string]struct { cro fleetv1alpha1.ClusterResourceOverride croList *fleetv1alpha1.ClusterResourceOverrideList @@ -345,7 +357,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, }, croList: &fleetv1alpha1.ClusterResourceOverrideList{}, - wantErrMsg: apiErrors.NewAggregate([]error{fmt.Errorf("resource selector %+v already exists, and must be unique", + wantErrMsg: apierrors.NewAggregate([]error{fmt.Errorf("resource selector %+v already exists, and must be unique", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "kind", Name: "example"}), fmt.Errorf("label selector is not supported for resource selection %+v", fleetv1beta1.ClusterResourceSelector{Group: "group", Version: "v1", Kind: "kind", @@ -472,7 +484,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, wantErrMsg: errors.New("labelSelector is required"), }, - "invalid cluster resource override - fail validateClusterResourceOverridePolicy with empty terms": { + "valid cluster resource override - empty cluster selector terms": { cro: fleetv1alpha1.ClusterResourceOverride{ Spec: fleetv1alpha1.ClusterResourceOverrideSpec{ Policy: &fleetv1alpha1.OverridePolicy{ @@ -481,12 +493,13 @@ func TestValidateClusterResourceOverride(t *testing.T) { ClusterSelector: &fleetv1beta1.ClusterSelector{ ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{}, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, }, }, - wantErrMsg: errors.New("clusterSelector must have at least one term"), + wantErrMsg: nil, }, "valid cluster resource override - empty match labels & match expressions": { cro: fleetv1alpha1.ClusterResourceOverride{ @@ -501,6 +514,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, { ClusterSelector: &fleetv1beta1.ClusterSelector{ @@ -510,6 +524,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, @@ -549,6 +564,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, { ClusterSelector: &fleetv1beta1.ClusterSelector{ @@ -569,6 +585,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, @@ -609,7 +626,9 @@ func TestValidateClusterResourceOverride(t *testing.T) { Spec: fleetv1alpha1.ClusterResourceOverrideSpec{ Policy: &fleetv1alpha1.OverridePolicy{ OverrideRules: []fleetv1alpha1.OverrideRule{ - {}, + { + JSONPatchOverrides: validJSONPatchOverrides, + }, }, }, }, @@ -674,6 +693,7 @@ func TestValidateClusterResourceOverride(t *testing.T) { {}, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, @@ -681,6 +701,75 @@ func TestValidateClusterResourceOverride(t *testing.T) { }, wantErrMsg: errors.New("labelSelector is required"), }, + "invalid cluster resource override - multiple invalid override paths, 1 valid": { + cro: fleetv1alpha1.ClusterResourceOverride{ + Spec: fleetv1alpha1.ClusterResourceOverrideSpec{ + Policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, + }, + }, + }, + }, + JSONPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, + Path: "/Kind", + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, + Path: "spec.resourceSelectors/matchExpressions", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-reason"`)}, + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, + Path: "/status", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + }, + }, + }, + }, + }, + }, + wantErrMsg: apierrors.NewAggregate([]error{ + fmt.Errorf("invalid JSONPatchOverride %s: path must start with /", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, Path: "spec.resourceSelectors/matchExpressions", Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}}), + fmt.Errorf("invalid JSONPatchOverride %s: path cannot be empty", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, Path: "", Value: apiextensionsv1.JSON{Raw: []byte(`"new-reason"`)}}), + fmt.Errorf("invalid JSONPatchOverride %s: cannot override status fields", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, Path: "/status", Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}}), + fmt.Errorf("invalid JSONPatchOverride %s: remove operation cannot have value", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, Path: "/status", Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}}), + }), + }, + "valid cluster resource override - empty cluster selector": { + cro: fleetv1alpha1.ClusterResourceOverride{ + Spec: fleetv1alpha1.ClusterResourceOverrideSpec{ + Policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{}, + JSONPatchOverrides: validJSONPatchOverrides, + }, + }, + }, + }, + }, + wantErrMsg: nil, + }, } for testName, tt := range tests { t.Run(testName, func(t *testing.T) { diff --git a/pkg/utils/validator/resourceoverride.go b/pkg/utils/validator/resourceoverride.go index 555efbeaf..362deb62d 100644 --- a/pkg/utils/validator/resourceoverride.go +++ b/pkg/utils/validator/resourceoverride.go @@ -7,9 +7,11 @@ Licensed under the MIT license. package validator import ( + "errors" "fmt" + "strings" - "k8s.io/apimachinery/pkg/util/errors" + apierrors "k8s.io/apimachinery/pkg/util/errors" fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" ) @@ -29,14 +31,13 @@ func ValidateResourceOverride(ro fleetv1alpha1.ResourceOverride, roList *fleetv1 allErr = append(allErr, err) } - // Check if override rule is using label selector if ro.Spec.Policy != nil { if err := validateOverridePolicy(ro.Spec.Policy); err != nil { allErr = append(allErr, err) } } - return errors.NewAggregate(allErr) + return apierrors.NewAggregate(allErr) } // validateResourceSelectors checks if override is selecting a unique resource. @@ -50,7 +51,7 @@ func validateResourceSelectors(ro fleetv1alpha1.ResourceOverride) error { } selectorMap[selector] = true } - return errors.NewAggregate(allErr) + return apierrors.NewAggregate(allErr) } // validateResourceOverrideResourceLimit checks if there is only 1 resource override per resource, @@ -80,31 +81,82 @@ func validateResourceOverrideResourceLimit(ro fleetv1alpha1.ResourceOverride, ro allErr = append(allErr, fmt.Errorf("invalid resource selector %+v: the resource has been selected by both %v and %v, which is not supported", roSelector, ro.GetName(), overrideMap[roSelector])) } } - return errors.NewAggregate(allErr) + return apierrors.NewAggregate(allErr) } // validateOverridePolicy checks if override rule is selecting resource by name. func validateOverridePolicy(policy *fleetv1alpha1.OverridePolicy) error { allErr := make([]error, 0) for _, rule := range policy.OverrideRules { - if rule.ClusterSelector == nil { - continue - } else if len(rule.ClusterSelector.ClusterSelectorTerms) == 0 { - allErr = append(allErr, fmt.Errorf("clusterSelector must have at least one term")) + if rule.ClusterSelector != nil { + for _, selector := range rule.ClusterSelector.ClusterSelectorTerms { + // Check that only label selector is supported + if selector.PropertySelector != nil || selector.PropertySorter != nil { + allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: only labelSelector is supported", selector)) + continue + } + if selector.LabelSelector == nil { + allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: labelSelector is required", selector)) + } else if err := validateLabelSelector(selector.LabelSelector, "cluster selector"); err != nil { + allErr = append(allErr, err) + } + } } - for _, selector := range rule.ClusterSelector.ClusterSelectorTerms { - // Check that only label selector is supported - if selector.PropertySelector != nil || selector.PropertySorter != nil { - allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: only labelSelector is supported", selector)) - continue - } - if selector.LabelSelector == nil { - allErr = append(allErr, fmt.Errorf("invalid clusterSelector %v: labelSelector is required", selector)) - } else if err := validateLabelSelector(selector.LabelSelector, "cluster selector"); err != nil { - allErr = append(allErr, err) - } + if err := validateJSONPatchOverride(rule.JSONPatchOverrides); err != nil { + allErr = append(allErr, err) + } + } + return apierrors.NewAggregate(allErr) +} + +// validateJSONPatchOverride checks if JSON patch override is valid. +func validateJSONPatchOverride(jsonPatchOverrides []fleetv1alpha1.JSONPatchOverride) error { + if len(jsonPatchOverrides) == 0 { + return errors.New("invalid JSONPatchOverrides: JSONPatchOverrides cannot be empty") + } + + allErr := make([]error, 0) + for _, patch := range jsonPatchOverrides { + if err := validateJSONPatchOverridePath(patch.Path); err != nil { + allErr = append(allErr, fmt.Errorf("invalid JSONPatchOverride %s: %w", patch, err)) + } + + if patch.Operator == fleetv1alpha1.JSONPatchOverrideOpRemove && len(patch.Value.Raw) != 0 { + allErr = append(allErr, fmt.Errorf("invalid JSONPatchOverride %s: remove operation cannot have value", patch)) + } + } + return apierrors.NewAggregate(allErr) +} + +func validateJSONPatchOverridePath(path string) error { + if path == "" { + return fmt.Errorf("path cannot be empty") + } + + if !strings.HasPrefix(path, "/") { + return fmt.Errorf("path must start with /") + } + + // The path begins with a slash, and at least there will be two elements. + parts := strings.Split(path, "/")[1:] + switch parts[0] { + case "kind", "apiVersion": + return fmt.Errorf("cannot override typeMeta fields") + case "metadata": + if len(parts) == 1 { + return fmt.Errorf("cannot override field metadata") + } else if parts[1] != "annotations" && parts[1] != "labels" { + return fmt.Errorf("cannot override metadata fields except annotations and labels") + } + case "status": + return fmt.Errorf("cannot override status fields") + } + + for i := range parts { + if len(strings.TrimSpace(parts[i])) == 0 { + return fmt.Errorf("path cannot contain empty string") } } - return errors.NewAggregate(allErr) + return nil } diff --git a/pkg/utils/validator/resourceoverride_test.go b/pkg/utils/validator/resourceoverride_test.go index dcf9c234c..869d4eee3 100644 --- a/pkg/utils/validator/resourceoverride_test.go +++ b/pkg/utils/validator/resourceoverride_test.go @@ -6,7 +6,9 @@ import ( "strings" "testing" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apierrors "k8s.io/apimachinery/pkg/util/errors" fleetv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" @@ -193,31 +195,42 @@ func TestValidateResourceOverrideResourceLimit(t *testing.T) { } func TestValidateResourceOverride(t *testing.T) { - validPolicy := &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ + validClusterSelector := &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key": "value", - }, - }, - }, - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key1": "value1", - }, - }, - }, + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, + }, + }, + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key1": "value1", }, }, }, }, } + validJSONPatchOverrides := []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, + Path: "/metadata/labels/new-label", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + } + + validPolicy := &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: validClusterSelector, + JSONPatchOverrides: validJSONPatchOverrides, + }, + }, + } + tests := map[string]struct { ro fleetv1alpha1.ResourceOverride roList *fleetv1alpha1.ResourceOverrideList @@ -335,7 +348,7 @@ func TestValidateResourceOverride(t *testing.T) { roList: nil, wantErrMsg: nil, }, - "invalid cluster resource override - fail validateResourceOverridePolicy with unsupported type ": { + "invalid resource override - fail validateResourceOverridePolicy with unsupported type ": { ro: fleetv1alpha1.ResourceOverride{ Spec: fleetv1alpha1.ResourceOverrideSpec{ Policy: &fleetv1alpha1.OverridePolicy{ @@ -384,7 +397,22 @@ func TestValidateResourceOverride(t *testing.T) { }, wantErrMsg: errors.New("labelSelector is required"), }, - "invalid cluster resource override - fail validateResourceOverridePolicy with empty terms": { + "valid resource override - empty cluster selector": { + ro: fleetv1alpha1.ResourceOverride{ + Spec: fleetv1alpha1.ResourceOverrideSpec{ + Policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{}, + JSONPatchOverrides: validJSONPatchOverrides, + }, + }, + }, + }, + }, + wantErrMsg: nil, + }, + "valid resource override - cluster selector with empty terms": { ro: fleetv1alpha1.ResourceOverride{ Spec: fleetv1alpha1.ResourceOverrideSpec{ Policy: &fleetv1alpha1.OverridePolicy{ @@ -393,14 +421,15 @@ func TestValidateResourceOverride(t *testing.T) { ClusterSelector: &fleetv1beta1.ClusterSelector{ ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{}, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, }, }, - wantErrMsg: errors.New("clusterSelector must have at least one term"), + wantErrMsg: nil, }, - "valid cluster resource override - empty match labels & match expressions": { + "valid resource override - empty match labels & match expressions": { ro: fleetv1alpha1.ResourceOverride{ Spec: fleetv1alpha1.ResourceOverrideSpec{ Policy: &fleetv1alpha1.OverridePolicy{ @@ -413,6 +442,7 @@ func TestValidateResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, { ClusterSelector: &fleetv1beta1.ClusterSelector{ @@ -422,6 +452,7 @@ func TestValidateResourceOverride(t *testing.T) { }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, @@ -429,7 +460,7 @@ func TestValidateResourceOverride(t *testing.T) { }, wantErrMsg: nil, }, - "valid cluster resource override - no policy": { + "valid resource override - no policy": { ro: fleetv1alpha1.ResourceOverride{ Spec: fleetv1alpha1.ResourceOverrideSpec{ Policy: nil, @@ -437,6 +468,57 @@ func TestValidateResourceOverride(t *testing.T) { }, wantErrMsg: nil, }, + "invalid resource override - multiple invalid override paths": { + ro: fleetv1alpha1.ResourceOverride{ + Spec: fleetv1alpha1.ResourceOverrideSpec{ + Policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, + }, + }, + }, + }, + JSONPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, + Path: "/apiVersion", + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, + Path: "/metadata/annotations/0", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "/status/conditions/0/reason", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-reason"`)}, + }, + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "/////kind///", + Value: apiextensionsv1.JSON{Raw: []byte(`"value"`)}, + }, + }, + }, + }, + }, + }, + }, + wantErrMsg: apierrors.NewAggregate([]error{fmt.Errorf("invalid JSONPatchOverride %s: cannot override typeMeta fields", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, Path: "/apiVersion"}), + fmt.Errorf("invalid JSONPatchOverride %s: cannot override status fields", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, Path: "/status/conditions/0/reason", Value: apiextensionsv1.JSON{Raw: []byte(`"new-reason"`)}}), + fmt.Errorf("invalid JSONPatchOverride %s: path cannot contain empty string", + fleetv1alpha1.JSONPatchOverride{Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, Path: "/////kind///", Value: apiextensionsv1.JSON{Raw: []byte(`"value"`)}}), + }), + }, } for testName, tt := range tests { t.Run(testName, func(t *testing.T) { @@ -453,78 +535,80 @@ func TestValidateResourceOverride(t *testing.T) { } func TestValidateOverridePolicy(t *testing.T) { + validJSONPatchOverrides := []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpAdd, + Path: "/metadata/labels/new-label", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + } + tests := map[string]struct { - ro fleetv1alpha1.ResourceOverride + policy *fleetv1alpha1.OverridePolicy wantErrMsg error }{ "all label selectors": { - ro: fleetv1alpha1.ResourceOverride{ - Spec: fleetv1alpha1.ResourceOverrideSpec{ - Policy: &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ - { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key": "value", - }, - }, + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", }, - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key1": "value1", - }, - }, + }, + }, + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key1": "value1", }, }, }, }, - { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key2": "value2", - }, - }, + }, + JSONPatchOverrides: validJSONPatchOverrides, + }, + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key2": "value2", }, - { - LabelSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "key3": "value3", - }, - }, + }, + }, + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key3": "value3", }, }, }, }, }, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, wantErrMsg: nil, }, - "unsupported selector type": { - ro: fleetv1alpha1.ResourceOverride{ - Spec: fleetv1alpha1.ResourceOverrideSpec{ - Policy: &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ - { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - { - PropertySelector: &fleetv1beta1.PropertySelector{ - MatchExpressions: []fleetv1beta1.PropertySelectorRequirement{ - { - Name: "example", - Operator: fleetv1beta1.PropertySelectorGreaterThanOrEqualTo, - Values: []string{"1"}, - }, - }, + "unsupported selector type - property selector": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + PropertySelector: &fleetv1beta1.PropertySelector{ + MatchExpressions: []fleetv1beta1.PropertySelectorRequirement{ + { + Name: "example", + Operator: fleetv1beta1.PropertySelectorGreaterThanOrEqualTo, + Values: []string{"1"}, }, }, }, @@ -537,39 +621,120 @@ func TestValidateOverridePolicy(t *testing.T) { wantErrMsg: fmt.Errorf("only labelSelector is supported"), }, "no cluster selector": { - ro: fleetv1alpha1.ResourceOverride{ - Spec: fleetv1alpha1.ResourceOverrideSpec{ - Policy: &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ - {}, - }, + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + JSONPatchOverrides: validJSONPatchOverrides, + }, + }, + }, + wantErrMsg: nil, + }, + "empty cluster selector": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{}, + JSONPatchOverrides: validJSONPatchOverrides, }, }, }, wantErrMsg: nil, }, "nil label selector": { - ro: fleetv1alpha1.ResourceOverride{ - Spec: fleetv1alpha1.ResourceOverrideSpec{ - Policy: &fleetv1alpha1.OverridePolicy{ - OverrideRules: []fleetv1alpha1.OverrideRule{ - { - ClusterSelector: &fleetv1beta1.ClusterSelector{ - ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ - {}, + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + {}, + }, + }, + }, + }, + }, + wantErrMsg: errors.New("labelSelector is required"), + }, + "nil JSONPatchOverride": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, }, }, }, }, + JSONPatchOverrides: nil, }, }, }, - wantErrMsg: errors.New("labelSelector is required"), + wantErrMsg: errors.New("JSONPatchOverrides cannot be empty"), + }, + "empty JSONPatchOverrides": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{ + ClusterSelectorTerms: []fleetv1beta1.ClusterSelectorTerm{ + { + LabelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "key": "value", + }, + }, + }, + }, + }, + JSONPatchOverrides: []fleetv1alpha1.JSONPatchOverride{}, + }, + }, + }, + wantErrMsg: errors.New("JSONPatchOverrides cannot be empty"), + }, + "invalid JSONPatchOverridesPath": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{}, + JSONPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "/metadata/finalizers", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + }, + }, + }, + }, + wantErrMsg: errors.New("cannot override metadata fields except annotations and labels"), + }, + "invalid JSONPatchOverride": { + policy: &fleetv1alpha1.OverridePolicy{ + OverrideRules: []fleetv1alpha1.OverrideRule{ + { + ClusterSelector: &fleetv1beta1.ClusterSelector{}, + JSONPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, + Path: "/apiVersionabc", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-value"`)}, + }, + }, + }, + }, + }, + wantErrMsg: errors.New("remove operation cannot have value"), }, } for testName, tt := range tests { t.Run(testName, func(t *testing.T) { - got := validateOverridePolicy(tt.ro.Spec.Policy) + got := validateOverridePolicy(tt.policy) if gotErr, wantErr := got != nil, tt.wantErrMsg != nil; gotErr != wantErr { t.Fatalf("validateOverridePolicy() = %v, want %v", got, tt.wantErrMsg) } @@ -580,3 +745,149 @@ func TestValidateOverridePolicy(t *testing.T) { }) } } + +func TestValidateJSONPatchOverride(t *testing.T) { + tests := map[string]struct { + jsonPatchOverrides []fleetv1alpha1.JSONPatchOverride + wantErrMsg error + }{ + "valid json patch override": { + jsonPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "/spec/clusterResourceSelector/kind", + Value: apiextensionsv1.JSON{Raw: []byte(`"ClusterRole"`)}, + }, + }, + wantErrMsg: nil, + }, + "invalid json patch override - invalid remove operation": { + jsonPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpRemove, + Path: "/spec/clusterResourceSelector/kind", + Value: apiextensionsv1.JSON{Raw: []byte(`"ClusterRole"`)}, + }, + }, + wantErrMsg: errors.New("remove operation cannot have value"), + }, + "invalid json patch override - nil jsonPatchOverrides": { + jsonPatchOverrides: nil, + wantErrMsg: errors.New("JSONPatchOverrides cannot be empty"), + }, + "invalid json patch override - empty jsonPatchOverrides": { + jsonPatchOverrides: []fleetv1alpha1.JSONPatchOverride{}, + wantErrMsg: errors.New("JSONPatchOverrides cannot be empty"), + }, + "invalid json patch override - invalid path": { + jsonPatchOverrides: []fleetv1alpha1.JSONPatchOverride{ + { + Operator: fleetv1alpha1.JSONPatchOverrideOpReplace, + Path: "/status/conditions/0/reason", + Value: apiextensionsv1.JSON{Raw: []byte(`"new-reason"`)}, + }, + }, + wantErrMsg: errors.New("cannot override status fields"), + }, + } + for testName, tt := range tests { + t.Run(testName, func(t *testing.T) { + got := validateJSONPatchOverride(tt.jsonPatchOverrides) + if gotErr, wantErr := got != nil, tt.wantErrMsg != nil; gotErr != wantErr { + t.Fatalf("validateJSONPatchOverride() = %v, want %v", got, tt.wantErrMsg) + } + + if got != nil && !strings.Contains(got.Error(), tt.wantErrMsg.Error()) { + t.Errorf("validateJSONPatchOverride() = %v, want %v", got, tt.wantErrMsg) + } + }) + } +} + +func TestValidateJSONPatchOverridePath(t *testing.T) { + tests := map[string]struct { + path string + wantErrMsg error + }{ + "valid json patch override path": { + path: "/spec/clusterResourceSelector/kind", + wantErrMsg: nil, + }, + "invalid json patch override path- cannot override typeMeta fields (kind)": { + path: "/kind", + wantErrMsg: errors.New("cannot override typeMeta fields"), + }, + "invalid json patch override path - cannot override typeMeta fields (apiVersion)": { + path: "/apiVersion", + wantErrMsg: errors.New("cannot override typeMeta fields"), + }, + "invalid json patch override path - cannot override metadata fields": { + path: "/metadata/finalizers/0", + wantErrMsg: errors.New("cannot override metadata fields"), + }, + "invalid json patch override path - cannot override any status field": { + path: "/status/conditions/0/reason", + wantErrMsg: errors.New("cannot override status fields"), + }, + "valid json patch override path - correct metadata field": { + path: "/metadata/annotations/new-annotation", + wantErrMsg: nil, + }, + "valid json patch override path - apiVersion used as label": { + path: "/metadata/labels/apiVersion", + wantErrMsg: nil, + }, + "valid json patch override path- case sensitive check": { + path: "/Kind", + wantErrMsg: nil, + }, + "invalid json patch override path - cannot override status": { + path: "/status", + wantErrMsg: errors.New("cannot override status fields"), + }, + "valid json patch override path- apiVersion within path": { + path: "/apiVersionabc", + wantErrMsg: nil, + }, + "invalid json patch override path - empty path": { + path: "", + wantErrMsg: errors.New("path cannot be empty"), + }, + "invalid json patch override path - slashes only": { + path: "/////", + wantErrMsg: errors.New("path cannot contain empty string"), + }, + "invalid json patch override path - path must start with /": { + path: "spec.resourceSelectors/selectors/0/name", + wantErrMsg: errors.New("path must start with /"), + }, + "invalid json patch override path - cannot override metadata fields (finalizer)": { + path: "/metadata/finalizers", + wantErrMsg: errors.New("cannot override metadata fields except annotations and labels"), + }, + "invalid json patch override path - invalid metadata field": { + path: "/metadata/annotationsabc", + wantErrMsg: errors.New("cannot override metadata fields"), + }, + "invalid json patch override path - contains empty string": { + path: "/spec/clusterNames///member-1", + wantErrMsg: errors.New("path cannot contain empty string"), + }, + "invalid json patch override path - metadata field": { + path: "/metadata", + wantErrMsg: errors.New("cannot override field metadata"), + }, + } + for testName, tt := range tests { + t.Run(testName, func(t *testing.T) { + got := validateJSONPatchOverridePath(tt.path) + if gotErr, wantErr := got != nil, tt.wantErrMsg != nil; gotErr != wantErr { + t.Fatalf("validateJSONPatchOverridePath() = %v, want %v", got, tt.wantErrMsg) + } + + if got != nil && !strings.Contains(got.Error(), tt.wantErrMsg.Error()) { + t.Errorf("validateJSONPatchOverridePath() = %v, want %v", got, tt.wantErrMsg) + } + }) + } +}