Skip to content

Commit

Permalink
feat: update validation webhook for overrides (Azure#752)
Browse files Browse the repository at this point in the history
  • Loading branch information
@britaniar
britaniar authored Apr 19, 2024
1 parent 0ec0b63 commit d1d40da
Show file tree
Hide file tree
Showing 4 changed files with 588 additions and 137 deletions.
1 change: 0 additions & 1 deletion pkg/utils/validator/clusterresourceoverride.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
137 changes: 113 additions & 24 deletions pkg/utils/validator/clusterresourceoverride_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"})}),
},
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -501,6 +514,7 @@ func TestValidateClusterResourceOverride(t *testing.T) {
},
},
},
JSONPatchOverrides: validJSONPatchOverrides,
},
{
ClusterSelector: &fleetv1beta1.ClusterSelector{
Expand All @@ -510,6 +524,7 @@ func TestValidateClusterResourceOverride(t *testing.T) {
},
},
},
JSONPatchOverrides: validJSONPatchOverrides,
},
},
},
Expand Down Expand Up @@ -549,6 +564,7 @@ func TestValidateClusterResourceOverride(t *testing.T) {
},
},
},
JSONPatchOverrides: validJSONPatchOverrides,
},
{
ClusterSelector: &fleetv1beta1.ClusterSelector{
Expand All @@ -569,6 +585,7 @@ func TestValidateClusterResourceOverride(t *testing.T) {
},
},
},
JSONPatchOverrides: validJSONPatchOverrides,
},
},
},
Expand Down Expand Up @@ -609,7 +626,9 @@ func TestValidateClusterResourceOverride(t *testing.T) {
Spec: fleetv1alpha1.ClusterResourceOverrideSpec{
Policy: &fleetv1alpha1.OverridePolicy{
OverrideRules: []fleetv1alpha1.OverrideRule{
{},
{
JSONPatchOverrides: validJSONPatchOverrides,
},
},
},
},
Expand Down Expand Up @@ -674,13 +693,83 @@ func TestValidateClusterResourceOverride(t *testing.T) {
{},
},
},
JSONPatchOverrides: validJSONPatchOverrides,
},
},
},
},
},
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) {
Expand Down
94 changes: 73 additions & 21 deletions pkg/utils/validator/resourceoverride.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
Loading

0 comments on commit d1d40da

Please sign in to comment.