From 1b33ef882befb031cf1cf4269a1d0c095c6818ea Mon Sep 17 00:00:00 2001 From: carlory Date: Tue, 29 Nov 2022 10:28:34 +0800 Subject: [PATCH] move validation funcs into util/validation Signed-off-by: carlory --- pkg/util/helper/policy.go | 34 ---------- pkg/util/helper/policy_test.go | 62 ------------------ pkg/util/validation/validation.go | 33 ++++++++++ pkg/util/validation/validation_test.go | 63 +++++++++++++++++++ .../clusterpropagationpolicy/validating.go | 3 +- pkg/webhook/propagationpolicy/validating.go | 3 +- 6 files changed, 98 insertions(+), 100 deletions(-) diff --git a/pkg/util/helper/policy.go b/pkg/util/helper/policy.go index 897e25f87..4867f6c93 100644 --- a/pkg/util/helper/policy.go +++ b/pkg/util/helper/policy.go @@ -2,11 +2,9 @@ package helper import ( "encoding/json" - "fmt" discoveryv1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -30,38 +28,6 @@ func SetDefaultSpreadConstraints(spreadConstraints []policyv1alpha1.SpreadConstr } } -// ValidateSpreadConstraint tests if the constraints is valid. -func ValidateSpreadConstraint(spreadConstraints []policyv1alpha1.SpreadConstraint) error { - spreadByFields := sets.NewString() - - for _, constraint := range spreadConstraints { - // SpreadByField and SpreadByLabel should not co-exist - if len(constraint.SpreadByField) > 0 && len(constraint.SpreadByLabel) > 0 { - return fmt.Errorf("invalid constraints: SpreadByLabel(%s) should not co-exist with spreadByField(%s)", constraint.SpreadByLabel, constraint.SpreadByField) - } - - // If MaxGroups provided, it should greater or equal than MinGroups. - if constraint.MaxGroups > 0 && constraint.MaxGroups < constraint.MinGroups { - return fmt.Errorf("maxGroups(%d) lower than minGroups(%d) is not allowed", constraint.MaxGroups, constraint.MinGroups) - } - - if len(constraint.SpreadByField) > 0 { - spreadByFields.Insert(string(constraint.SpreadByField)) - } - } - - if spreadByFields.Len() > 0 { - // If one of spread constraints are using 'SpreadByField', the 'SpreadByFieldCluster' must be included. - // For example, when using 'SpreadByFieldRegion' to specify region groups, at the meantime, you must use - // 'SpreadByFieldCluster' to specify how many clusters should be selected. - if !spreadByFields.Has(string(policyv1alpha1.SpreadByFieldCluster)) { - return fmt.Errorf("the cluster spread constraint must be enabled in one of the constraints in case of SpreadByField is enabled") - } - } - - return nil -} - // IsDependentOverridesPresent checks if a PropagationPolicy's dependent OverridePolicy all exist. func IsDependentOverridesPresent(c client.Client, policy *policyv1alpha1.PropagationPolicy) (bool, error) { ns := policy.Namespace diff --git a/pkg/util/helper/policy_test.go b/pkg/util/helper/policy_test.go index 521341bf8..815be3fa0 100644 --- a/pkg/util/helper/policy_test.go +++ b/pkg/util/helper/policy_test.go @@ -2,7 +2,6 @@ package helper import ( "context" - "fmt" "reflect" "testing" @@ -60,67 +59,6 @@ func TestSetDefaultSpreadConstraints(t *testing.T) { } } -func TestValidateSpreadConstraint(t *testing.T) { - tests := []struct { - name string - spreadConstraint []policyv1alpha1.SpreadConstraint - expectedErr error - }{ - { - name: "spreadByLabel co-exist with spreadByField", - spreadConstraint: []policyv1alpha1.SpreadConstraint{ - { - SpreadByField: policyv1alpha1.SpreadByFieldCluster, - SpreadByLabel: "foo", - }, - }, - expectedErr: fmt.Errorf("invalid constraints: SpreadByLabel(foo) should not co-exist with spreadByField(cluster)"), - }, - { - name: "maxGroups lower than minGroups", - spreadConstraint: []policyv1alpha1.SpreadConstraint{ - { - MaxGroups: 1, - MinGroups: 2, - }, - }, - expectedErr: fmt.Errorf("maxGroups(1) lower than minGroups(2) is not allowed"), - }, - { - name: "spreadByFieldCluster must be included if using spreadByField", - spreadConstraint: []policyv1alpha1.SpreadConstraint{ - { - SpreadByField: policyv1alpha1.SpreadByFieldRegion, - }, - }, - expectedErr: fmt.Errorf("the cluster spread constraint must be enabled in one of the constraints in case of SpreadByField is enabled"), - }, - { - name: "validate success", - spreadConstraint: []policyv1alpha1.SpreadConstraint{ - { - MaxGroups: 2, - MinGroups: 1, - SpreadByField: policyv1alpha1.SpreadByFieldRegion, - }, - { - SpreadByField: policyv1alpha1.SpreadByFieldCluster, - }, - }, - expectedErr: nil, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := ValidateSpreadConstraint(tt.spreadConstraint) - if !reflect.DeepEqual(err, tt.expectedErr) { - t.Errorf("expected: %v, but got %v", tt.expectedErr, err) - } - }) - } -} - func TestIsDependentOverridesPresent(t *testing.T) { tests := []struct { name string diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 1b4f30338..552ed8d6f 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -4,6 +4,7 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" "github.com/karmada-io/karmada/pkg/util" @@ -35,6 +36,38 @@ func ValidatePolicyFieldSelector(fieldSelector *policyv1alpha1.FieldSelector) er return nil } +// ValidateSpreadConstraint tests if the constraints is valid. +func ValidateSpreadConstraint(spreadConstraints []policyv1alpha1.SpreadConstraint) error { + spreadByFields := sets.NewString() + + for _, constraint := range spreadConstraints { + // SpreadByField and SpreadByLabel should not co-exist + if len(constraint.SpreadByField) > 0 && len(constraint.SpreadByLabel) > 0 { + return fmt.Errorf("invalid constraints: SpreadByLabel(%s) should not co-exist with spreadByField(%s)", constraint.SpreadByLabel, constraint.SpreadByField) + } + + // If MaxGroups provided, it should greater or equal than MinGroups. + if constraint.MaxGroups > 0 && constraint.MaxGroups < constraint.MinGroups { + return fmt.Errorf("maxGroups(%d) lower than minGroups(%d) is not allowed", constraint.MaxGroups, constraint.MinGroups) + } + + if len(constraint.SpreadByField) > 0 { + spreadByFields.Insert(string(constraint.SpreadByField)) + } + } + + if spreadByFields.Len() > 0 { + // If one of spread constraints are using 'SpreadByField', the 'SpreadByFieldCluster' must be included. + // For example, when using 'SpreadByFieldRegion' to specify region groups, at the meantime, you must use + // 'SpreadByFieldCluster' to specify how many clusters should be selected. + if !spreadByFields.Has(string(policyv1alpha1.SpreadByFieldCluster)) { + return fmt.Errorf("the cluster spread constraint must be enabled in one of the constraints in case of SpreadByField is enabled") + } + } + + return nil +} + // ValidateOverrideSpec tests if the overrideRules and (overriders or targetCluster) co-exist func ValidateOverrideSpec(overrideSpec *policyv1alpha1.OverrideSpec) error { if overrideSpec == nil { diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 7e5f37c3e..dfb556176 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -1,6 +1,8 @@ package validation import ( + "fmt" + "reflect" "testing" corev1 "k8s.io/api/core/v1" @@ -117,6 +119,67 @@ func TestValidateOverrideSpec(t *testing.T) { } } +func TestValidateSpreadConstraint(t *testing.T) { + tests := []struct { + name string + spreadConstraint []policyv1alpha1.SpreadConstraint + expectedErr error + }{ + { + name: "spreadByLabel co-exist with spreadByField", + spreadConstraint: []policyv1alpha1.SpreadConstraint{ + { + SpreadByField: policyv1alpha1.SpreadByFieldCluster, + SpreadByLabel: "foo", + }, + }, + expectedErr: fmt.Errorf("invalid constraints: SpreadByLabel(foo) should not co-exist with spreadByField(cluster)"), + }, + { + name: "maxGroups lower than minGroups", + spreadConstraint: []policyv1alpha1.SpreadConstraint{ + { + MaxGroups: 1, + MinGroups: 2, + }, + }, + expectedErr: fmt.Errorf("maxGroups(1) lower than minGroups(2) is not allowed"), + }, + { + name: "spreadByFieldCluster must be included if using spreadByField", + spreadConstraint: []policyv1alpha1.SpreadConstraint{ + { + SpreadByField: policyv1alpha1.SpreadByFieldRegion, + }, + }, + expectedErr: fmt.Errorf("the cluster spread constraint must be enabled in one of the constraints in case of SpreadByField is enabled"), + }, + { + name: "validate success", + spreadConstraint: []policyv1alpha1.SpreadConstraint{ + { + MaxGroups: 2, + MinGroups: 1, + SpreadByField: policyv1alpha1.SpreadByFieldRegion, + }, + { + SpreadByField: policyv1alpha1.SpreadByFieldCluster, + }, + }, + expectedErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateSpreadConstraint(tt.spreadConstraint) + if !reflect.DeepEqual(err, tt.expectedErr) { + t.Errorf("expected: %v, but got %v", tt.expectedErr, err) + } + }) + } +} + func TestValidatePolicyFieldSelector(t *testing.T) { fakeProvider := []string{"fooCloud"} fakeRegion := []string{"fooRegion"} diff --git a/pkg/webhook/clusterpropagationpolicy/validating.go b/pkg/webhook/clusterpropagationpolicy/validating.go index cb6de13d1..99e163c6f 100644 --- a/pkg/webhook/clusterpropagationpolicy/validating.go +++ b/pkg/webhook/clusterpropagationpolicy/validating.go @@ -8,7 +8,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" - "github.com/karmada-io/karmada/pkg/util/helper" "github.com/karmada-io/karmada/pkg/util/validation" ) @@ -32,7 +31,7 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) } klog.V(2).Infof("Validating ClusterPropagationPolicy(%s) for request: %s", policy.Name, req.Operation) - if err := helper.ValidateSpreadConstraint(policy.Spec.Placement.SpreadConstraints); err != nil { + if err := validation.ValidateSpreadConstraint(policy.Spec.Placement.SpreadConstraints); err != nil { klog.Error(err) return admission.Denied(err.Error()) } diff --git a/pkg/webhook/propagationpolicy/validating.go b/pkg/webhook/propagationpolicy/validating.go index ba070c73e..9837ababf 100644 --- a/pkg/webhook/propagationpolicy/validating.go +++ b/pkg/webhook/propagationpolicy/validating.go @@ -8,7 +8,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1" - "github.com/karmada-io/karmada/pkg/util/helper" "github.com/karmada-io/karmada/pkg/util/validation" ) @@ -32,7 +31,7 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) } klog.V(2).Infof("Validating PropagationPolicy(%s/%s) for request: %s", policy.Namespace, policy.Name, req.Operation) - if err := helper.ValidateSpreadConstraint(policy.Spec.Placement.SpreadConstraints); err != nil { + if err := validation.ValidateSpreadConstraint(policy.Spec.Placement.SpreadConstraints); err != nil { klog.Error(err) return admission.Denied(err.Error()) }