diff --git a/pkg/framework/plugins/defaultevictor/validation.go b/pkg/framework/plugins/defaultevictor/validation.go index 9e2aa31a2..6dac513eb 100644 --- a/pkg/framework/plugins/defaultevictor/validation.go +++ b/pkg/framework/plugins/defaultevictor/validation.go @@ -17,16 +17,16 @@ import ( "fmt" "slices" - "k8s.io/klog/v2" - "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" ) func ValidateDefaultEvictorArgs(obj runtime.Object) error { args := obj.(*DefaultEvictorArgs) - + var allErrs []error if args.PriorityThreshold != nil && args.PriorityThreshold.Value != nil && len(args.PriorityThreshold.Name) > 0 { - return fmt.Errorf("priority threshold misconfigured, only one of priorityThreshold fields can be set") + allErrs = append(allErrs, fmt.Errorf("priority threshold misconfigured, only one of priorityThreshold fields can be set")) } if args.MinReplicas == 1 { @@ -35,7 +35,7 @@ func ValidateDefaultEvictorArgs(obj runtime.Object) error { if args.NoEvictionPolicy != "" { if args.NoEvictionPolicy != PreferredNoEvictionPolicy && args.NoEvictionPolicy != MandatoryNoEvictionPolicy { - return fmt.Errorf("noEvictionPolicy accepts only %q values", []NoEvictionPolicy{PreferredNoEvictionPolicy, MandatoryNoEvictionPolicy}) + allErrs = append(allErrs, fmt.Errorf("noEvictionPolicy accepts only %q values", []NoEvictionPolicy{PreferredNoEvictionPolicy, MandatoryNoEvictionPolicy})) } } @@ -46,35 +46,35 @@ func ValidateDefaultEvictorArgs(obj runtime.Object) error { // disallow mixing deprecated fields with PodProtections.ExtraEnabled and PodProtections.DefaultDisabled if hasDeprecatedFields && (len(args.PodProtections.ExtraEnabled) > 0 || len(args.PodProtections.DefaultDisabled) > 0) { - return fmt.Errorf("cannot use Deprecated fields alongside PodProtections.ExtraEnabled or PodProtections.DefaultDisabled") + allErrs = append(allErrs, fmt.Errorf("cannot use Deprecated fields alongside PodProtections.ExtraEnabled or PodProtections.DefaultDisabled")) } if len(args.PodProtections.ExtraEnabled) > 0 || len(args.PodProtections.DefaultDisabled) > 0 { for _, policy := range args.PodProtections.ExtraEnabled { if !slices.Contains(extraPodProtections, policy) { - return fmt.Errorf("invalid pod protection policy in ExtraEnabled: %q. Valid options are: %v", - string(policy), extraPodProtections) + allErrs = append(allErrs, fmt.Errorf("invalid pod protection policy in ExtraEnabled: %q. Valid options are: %v", + string(policy), extraPodProtections)) } } for _, policy := range args.PodProtections.DefaultDisabled { if !slices.Contains(defaultPodProtections, policy) { - return fmt.Errorf("invalid pod protection policy in DefaultDisabled: %q. Valid options are: %v", - string(policy), defaultPodProtections) + allErrs = append(allErrs, fmt.Errorf("invalid pod protection policy in DefaultDisabled: %q. Valid options are: %v", + string(policy), defaultPodProtections)) } } if hasDuplicates(args.PodProtections.DefaultDisabled) { - return fmt.Errorf("PodProtections.DefaultDisabled contains duplicate entries") + allErrs = append(allErrs, fmt.Errorf("PodProtections.DefaultDisabled contains duplicate entries")) } if hasDuplicates(args.PodProtections.ExtraEnabled) { - return fmt.Errorf("PodProtections.ExtraEnabled contains duplicate entries") + allErrs = append(allErrs, fmt.Errorf("PodProtections.ExtraEnabled contains duplicate entries")) } } - return nil + return utilerrors.NewAggregate(allErrs) } func hasDuplicates(slice []PodProtection) bool { diff --git a/pkg/framework/plugins/defaultevictor/validation_test.go b/pkg/framework/plugins/defaultevictor/validation_test.go index c50a1ec22..952b19a09 100644 --- a/pkg/framework/plugins/defaultevictor/validation_test.go +++ b/pkg/framework/plugins/defaultevictor/validation_test.go @@ -187,6 +187,17 @@ func TestValidateDefaultEvictorArgs(t *testing.T) { }, errInfo: fmt.Errorf(`PodProtections.DefaultDisabled contains duplicate entries`), }, + { + name: "Invalid DefaultDisabled duplicate and Invalid ExtraEnabled duplicate and passing invalid no eviction policy", + args: &DefaultEvictorArgs{ + NoEvictionPolicy: "invalid-no-eviction-policy", + PodProtections: PodProtections{ + ExtraEnabled: []PodProtection{PodsWithPVC, PodsWithPVC}, + DefaultDisabled: []PodProtection{PodsWithLocalStorage, PodsWithLocalStorage, PodsWithoutPDB}, + }, + }, + errInfo: fmt.Errorf(`[noEvictionPolicy accepts only ["Preferred" "Mandatory"] values, invalid pod protection policy in DefaultDisabled: "PodsWithoutPDB". Valid options are: [PodsWithLocalStorage SystemCriticalPods FailedBarePods DaemonSetPods], PodProtections.DefaultDisabled contains duplicate entries, PodProtections.ExtraEnabled contains duplicate entries]`), + }, } for _, testCase := range tests { diff --git a/pkg/framework/plugins/podlifetime/validation.go b/pkg/framework/plugins/podlifetime/validation.go index 4eb2fc828..fbe5b1aec 100644 --- a/pkg/framework/plugins/podlifetime/validation.go +++ b/pkg/framework/plugins/podlifetime/validation.go @@ -18,29 +18,31 @@ package podlifetime import ( "fmt" - - "k8s.io/apimachinery/pkg/runtime" + "sort" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" ) // ValidatePodLifeTimeArgs validates PodLifeTime arguments func ValidatePodLifeTimeArgs(obj runtime.Object) error { args := obj.(*PodLifeTimeArgs) + var allErrs []error if args.MaxPodLifeTimeSeconds == nil { - return fmt.Errorf("MaxPodLifeTimeSeconds not set") + allErrs = append(allErrs, fmt.Errorf("MaxPodLifeTimeSeconds not set")) } // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } podLifeTimeAllowedStates := sets.New( @@ -72,8 +74,10 @@ func ValidatePodLifeTimeArgs(obj runtime.Object) error { ) if !podLifeTimeAllowedStates.HasAll(args.States...) { - return fmt.Errorf("states must be one of %v", podLifeTimeAllowedStates.UnsortedList()) + allowed := podLifeTimeAllowedStates.UnsortedList() + sort.Strings(allowed) + allErrs = append(allErrs, fmt.Errorf("states must be one of %v", allowed)) } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/podlifetime/validation_test.go b/pkg/framework/plugins/podlifetime/validation_test.go index 9c93be989..5536432aa 100644 --- a/pkg/framework/plugins/podlifetime/validation_test.go +++ b/pkg/framework/plugins/podlifetime/validation_test.go @@ -17,6 +17,7 @@ limitations under the License. package podlifetime import ( + "fmt" "testing" v1 "k8s.io/api/core/v1" @@ -26,7 +27,7 @@ func TestValidateRemovePodLifeTimeArgs(t *testing.T) { testCases := []struct { description string args *PodLifeTimeArgs - expectError bool + errInfo error }{ { description: "valid arg, no errors", @@ -34,7 +35,6 @@ func TestValidateRemovePodLifeTimeArgs(t *testing.T) { MaxPodLifeTimeSeconds: func(i uint) *uint { return &i }(1), States: []string{string(v1.PodRunning)}, }, - expectError: false, }, { description: "Pod Status Reasons Succeeded or Failed", @@ -42,7 +42,6 @@ func TestValidateRemovePodLifeTimeArgs(t *testing.T) { MaxPodLifeTimeSeconds: func(i uint) *uint { return &i }(1), States: []string{string(v1.PodSucceeded), string(v1.PodFailed)}, }, - expectError: false, }, { description: "Pod Status Reasons CrashLoopBackOff ", @@ -50,31 +49,41 @@ func TestValidateRemovePodLifeTimeArgs(t *testing.T) { MaxPodLifeTimeSeconds: func(i uint) *uint { return &i }(1), States: []string{"CrashLoopBackOff"}, }, - expectError: false, }, { description: "nil MaxPodLifeTimeSeconds arg, expects errors", args: &PodLifeTimeArgs{ MaxPodLifeTimeSeconds: nil, }, - expectError: true, + errInfo: fmt.Errorf("MaxPodLifeTimeSeconds not set"), }, { description: "invalid pod state arg, expects errors", args: &PodLifeTimeArgs{ - States: []string{string(v1.NodeRunning)}, + MaxPodLifeTimeSeconds: func(i uint) *uint { return &i }(1), + States: []string{string("InvalidState")}, }, - expectError: true, + errInfo: fmt.Errorf("states must be one of [ContainerCreating CrashLoopBackOff CreateContainerConfigError CreateContainerError ErrImagePull Failed ImagePullBackOff InvalidImageName NodeAffinity NodeLost Pending PodInitializing Running Shutdown Succeeded UnexpectedAdmissionError Unknown]"), + }, + { + description: "nil MaxPodLifeTimeSeconds arg and invalid pod state arg, expects errors", + args: &PodLifeTimeArgs{ + MaxPodLifeTimeSeconds: nil, + States: []string{string("InvalidState")}, + }, + errInfo: fmt.Errorf("[MaxPodLifeTimeSeconds not set, states must be one of [ContainerCreating CrashLoopBackOff CreateContainerConfigError CreateContainerError ErrImagePull Failed ImagePullBackOff InvalidImageName NodeAffinity NodeLost Pending PodInitializing Running Shutdown Succeeded UnexpectedAdmissionError Unknown]]"), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidatePodLifeTimeArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidatePodLifeTimeArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removeduplicates/validation.go b/pkg/framework/plugins/removeduplicates/validation.go index a2e359e44..fbafe2fe4 100644 --- a/pkg/framework/plugins/removeduplicates/validation.go +++ b/pkg/framework/plugins/removeduplicates/validation.go @@ -17,14 +17,16 @@ import ( "fmt" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) func ValidateRemoveDuplicatesArgs(obj runtime.Object) error { args := obj.(*RemoveDuplicatesArgs) + var allErrs []error // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removeduplicates/validation_test.go b/pkg/framework/plugins/removeduplicates/validation_test.go index 2fb4a9cdd..bf9bf1710 100644 --- a/pkg/framework/plugins/removeduplicates/validation_test.go +++ b/pkg/framework/plugins/removeduplicates/validation_test.go @@ -1,6 +1,7 @@ package removeduplicates import ( + "fmt" "testing" "sigs.k8s.io/descheduler/pkg/api" @@ -11,6 +12,7 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { description string args *RemoveDuplicatesArgs expectError bool + errInfo error }{ { description: "valid namespace args, no errors", @@ -20,7 +22,6 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { Include: []string{"default"}, }, }, - expectError: false, }, { description: "invalid namespaces args, expects error", @@ -31,17 +32,19 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { Exclude: []string{"kube-system"}, }, }, - expectError: true, + errInfo: fmt.Errorf("only one of Include/Exclude namespaces can be set"), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemoveDuplicatesArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemoveDuplicatesArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removefailedpods/validation.go b/pkg/framework/plugins/removefailedpods/validation.go index 6d04065ba..830a21aac 100644 --- a/pkg/framework/plugins/removefailedpods/validation.go +++ b/pkg/framework/plugins/removefailedpods/validation.go @@ -18,21 +18,23 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) // ValidateRemoveFailedPodsArgs validates RemoveFailedPods arguments func ValidateRemoveFailedPodsArgs(obj runtime.Object) error { args := obj.(*RemoveFailedPodsArgs) + var allErrs []error // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removefailedpods/validation_test.go b/pkg/framework/plugins/removefailedpods/validation_test.go index 9c79825b5..78cc8fb21 100644 --- a/pkg/framework/plugins/removefailedpods/validation_test.go +++ b/pkg/framework/plugins/removefailedpods/validation_test.go @@ -1,6 +1,7 @@ package removefailedpods import ( + "fmt" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,7 +13,7 @@ func TestValidateRemoveFailedPodsArgs(t *testing.T) { testCases := []struct { description string args *RemoveFailedPodsArgs - expectError bool + errInfo error }{ { description: "valid namespace args, no errors", @@ -24,7 +25,6 @@ func TestValidateRemoveFailedPodsArgs(t *testing.T) { Reasons: []string{"ReasonDoesNotMatch"}, MinPodLifetimeSeconds: &oneHourPodLifetimeSeconds, }, - expectError: false, }, { description: "invalid namespaces args, expects error", @@ -34,7 +34,7 @@ func TestValidateRemoveFailedPodsArgs(t *testing.T) { Exclude: []string{"kube-system"}, }, }, - expectError: true, + errInfo: fmt.Errorf(`only one of Include/Exclude namespaces can be set`), }, { description: "valid label selector args, no errors", @@ -43,7 +43,6 @@ func TestValidateRemoveFailedPodsArgs(t *testing.T) { MatchLabels: map[string]string{"role.kubernetes.io/node": ""}, }, }, - expectError: false, }, { description: "invalid label selector args, expects errors", @@ -56,16 +55,19 @@ func TestValidateRemoveFailedPodsArgs(t *testing.T) { }, }, }, - expectError: true, + errInfo: fmt.Errorf(`failed to get label selectors from strategy's params: [key: Invalid value: "": name part must be non-empty; name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]'), values: Invalid value: []string(nil): for 'in', 'notin' operators, values set can't be empty]`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemoveFailedPodsArgs(tc.args) - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemoveFailedPodsArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation.go b/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation.go index e1935a43f..e9d4705b9 100644 --- a/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation.go +++ b/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation.go @@ -15,29 +15,32 @@ package removepodshavingtoomanyrestarts import ( "fmt" + "sort" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" ) // ValidateRemovePodsHavingTooManyRestartsArgs validates RemovePodsHavingTooManyRestarts arguments func ValidateRemovePodsHavingTooManyRestartsArgs(obj runtime.Object) error { args := obj.(*RemovePodsHavingTooManyRestartsArgs) + var allErrs []error // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } if args.PodRestartThreshold < 1 { - return fmt.Errorf("invalid PodsHavingTooManyRestarts threshold") + allErrs = append(allErrs, fmt.Errorf("invalid PodsHavingTooManyRestarts threshold")) } allowedStates := sets.New( @@ -49,8 +52,10 @@ func ValidateRemovePodsHavingTooManyRestartsArgs(obj runtime.Object) error { ) if !allowedStates.HasAll(args.States...) { - return fmt.Errorf("states must be one of %v", allowedStates.UnsortedList()) + allowed := allowedStates.UnsortedList() + sort.Strings(allowed) + allErrs = append(allErrs, fmt.Errorf("states must be one of %v", allowed)) } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation_test.go b/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation_test.go index 03578f308..5025cc45c 100644 --- a/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation_test.go +++ b/pkg/framework/plugins/removepodshavingtoomanyrestarts/validation_test.go @@ -17,6 +17,7 @@ limitations under the License. package removepodshavingtoomanyrestarts import ( + "fmt" "testing" v1 "k8s.io/api/core/v1" @@ -26,7 +27,7 @@ func TestValidateRemovePodsHavingTooManyRestartsArgs(t *testing.T) { testCases := []struct { description string args *RemovePodsHavingTooManyRestartsArgs - expectError bool + errInfo error }{ { description: "valid arg, no errors", @@ -34,14 +35,13 @@ func TestValidateRemovePodsHavingTooManyRestartsArgs(t *testing.T) { PodRestartThreshold: 1, States: []string{string(v1.PodRunning)}, }, - expectError: false, }, { description: "invalid PodRestartThreshold arg, expects errors", args: &RemovePodsHavingTooManyRestartsArgs{ PodRestartThreshold: 0, }, - expectError: true, + errInfo: fmt.Errorf(`invalid PodsHavingTooManyRestarts threshold`), }, { description: "invalid States arg, expects errors", @@ -49,7 +49,7 @@ func TestValidateRemovePodsHavingTooManyRestartsArgs(t *testing.T) { PodRestartThreshold: 1, States: []string{string(v1.PodFailed)}, }, - expectError: true, + errInfo: fmt.Errorf(`states must be one of [CrashLoopBackOff Running]`), }, { description: "allows CrashLoopBackOff state", @@ -57,17 +57,26 @@ func TestValidateRemovePodsHavingTooManyRestartsArgs(t *testing.T) { PodRestartThreshold: 1, States: []string{"CrashLoopBackOff"}, }, - expectError: false, + }, + { + description: "invalid PodRestartThreshold arg and invalid States arg, expects errors", + args: &RemovePodsHavingTooManyRestartsArgs{ + PodRestartThreshold: 0, + States: []string{string(v1.PodFailed)}, + }, + errInfo: fmt.Errorf(`[invalid PodsHavingTooManyRestarts threshold, states must be one of [CrashLoopBackOff Running]]`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemovePodsHavingTooManyRestartsArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemovePodsHavingTooManyRestartsArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation.go b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation.go index 514cdb8e4..10fb4c58a 100644 --- a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation.go +++ b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation.go @@ -18,21 +18,23 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) // ValidateRemovePodsViolatingInterPodAntiAffinityArgs validates ValidateRemovePodsViolatingInterPodAntiAffinity arguments func ValidateRemovePodsViolatingInterPodAntiAffinityArgs(obj runtime.Object) error { args := obj.(*RemovePodsViolatingInterPodAntiAffinityArgs) + var allErrs []error // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation_test.go b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation_test.go index 108cc1c9e..4a9f36263 100644 --- a/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation_test.go +++ b/pkg/framework/plugins/removepodsviolatinginterpodantiaffinity/validation_test.go @@ -1,6 +1,7 @@ package removepodsviolatinginterpodantiaffinity import ( + "fmt" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -11,7 +12,7 @@ func TestValidateRemovePodsViolatingInterPodAntiAffinityArgs(t *testing.T) { testCases := []struct { description string args *RemovePodsViolatingInterPodAntiAffinityArgs - expectError bool + errInfo error }{ { description: "valid namespace args, no errors", @@ -20,7 +21,6 @@ func TestValidateRemovePodsViolatingInterPodAntiAffinityArgs(t *testing.T) { Include: []string{"default"}, }, }, - expectError: false, }, { description: "invalid namespaces args, expects error", @@ -30,7 +30,7 @@ func TestValidateRemovePodsViolatingInterPodAntiAffinityArgs(t *testing.T) { Exclude: []string{"kube-system"}, }, }, - expectError: true, + errInfo: fmt.Errorf(`only one of Include/Exclude namespaces can be set`), }, { description: "valid label selector args, no errors", @@ -39,7 +39,6 @@ func TestValidateRemovePodsViolatingInterPodAntiAffinityArgs(t *testing.T) { MatchLabels: map[string]string{"role.kubernetes.io/node": ""}, }, }, - expectError: false, }, { description: "invalid label selector args, expects errors", @@ -52,16 +51,19 @@ func TestValidateRemovePodsViolatingInterPodAntiAffinityArgs(t *testing.T) { }, }, }, - expectError: true, + errInfo: fmt.Errorf(`failed to get label selectors from strategy's params: [key: Invalid value: "": name part must be non-empty; name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]'), values: Invalid value: []string(nil): for 'in', 'notin' operators, values set can't be empty]`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemovePodsViolatingInterPodAntiAffinityArgs(tc.args) - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemovePodsViolatingInterPodAntiAffinityArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation.go b/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation.go index 8c95291eb..670596e4e 100644 --- a/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation.go +++ b/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation.go @@ -21,25 +21,27 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) // ValidateRemovePodsViolatingNodeAffinityArgs validates RemovePodsViolatingNodeAffinity arguments func ValidateRemovePodsViolatingNodeAffinityArgs(obj runtime.Object) error { args := obj.(*RemovePodsViolatingNodeAffinityArgs) + var allErrs []error if args == nil || len(args.NodeAffinityType) == 0 { - return fmt.Errorf("nodeAffinityType needs to be set") + allErrs = append(allErrs, fmt.Errorf("nodeAffinityType needs to be set")) } // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation_test.go b/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation_test.go index b8106608c..0f7e62d46 100644 --- a/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation_test.go +++ b/pkg/framework/plugins/removepodsviolatingnodeaffinity/validation_test.go @@ -16,44 +16,72 @@ limitations under the License. package removepodsviolatingnodeaffinity -import "testing" +import ( + "fmt" + "testing" + + "sigs.k8s.io/descheduler/pkg/api" +) func TestValidateRemovePodsViolatingNodeAffinityArgs(t *testing.T) { testCases := []struct { description string args *RemovePodsViolatingNodeAffinityArgs - expectError bool + errInfo error }{ { description: "nil NodeAffinityType args, expects errors", args: &RemovePodsViolatingNodeAffinityArgs{ NodeAffinityType: nil, }, - expectError: true, + errInfo: fmt.Errorf(`nodeAffinityType needs to be set`), }, { description: "empty NodeAffinityType args, expects errors", args: &RemovePodsViolatingNodeAffinityArgs{ NodeAffinityType: []string{}, }, - expectError: true, + errInfo: fmt.Errorf(`nodeAffinityType needs to be set`), }, { description: "valid NodeAffinityType args, no errors", args: &RemovePodsViolatingNodeAffinityArgs{ NodeAffinityType: []string{"requiredDuringSchedulingIgnoredDuringExecution"}, }, - expectError: false, + }, + { + description: "invalid namespaces args, expects error", + args: &RemovePodsViolatingNodeAffinityArgs{ + NodeAffinityType: []string{"requiredDuringSchedulingIgnoredDuringExecution"}, + Namespaces: &api.Namespaces{ + Include: []string{"default"}, + Exclude: []string{"kube-system"}, + }, + }, + errInfo: fmt.Errorf(`only one of Include/Exclude namespaces can be set`), + }, + { + description: "nil NodeAffinityType args and invalid namespaces args, expects error", + args: &RemovePodsViolatingNodeAffinityArgs{ + NodeAffinityType: []string{}, + Namespaces: &api.Namespaces{ + Include: []string{"default"}, + Exclude: []string{"kube-system"}, + }, + }, + errInfo: fmt.Errorf(`[nodeAffinityType needs to be set, only one of Include/Exclude namespaces can be set]`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemovePodsViolatingNodeAffinityArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemovePodsViolatingNodeAffinityArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removepodsviolatingnodetaints/validation.go b/pkg/framework/plugins/removepodsviolatingnodetaints/validation.go index def7a06df..6493ea4cb 100644 --- a/pkg/framework/plugins/removepodsviolatingnodetaints/validation.go +++ b/pkg/framework/plugins/removepodsviolatingnodetaints/validation.go @@ -21,25 +21,27 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" ) // ValidateRemovePodsViolatingNodeTaintsArgs validates RemovePodsViolatingNodeTaints arguments func ValidateRemovePodsViolatingNodeTaintsArgs(obj runtime.Object) error { args := obj.(*RemovePodsViolatingNodeTaintsArgs) + var allErrs []error // At most one of include/exclude can be set if args.Namespaces != nil && len(args.Namespaces.Include) > 0 && len(args.Namespaces.Exclude) > 0 { - return fmt.Errorf("only one of Include/Exclude namespaces can be set") + allErrs = append(allErrs, fmt.Errorf("only one of Include/Exclude namespaces can be set")) } if args.LabelSelector != nil { if _, err := metav1.LabelSelectorAsSelector(args.LabelSelector); err != nil { - return fmt.Errorf("failed to get label selectors from strategy's params: %+v", err) + allErrs = append(allErrs, fmt.Errorf("failed to get label selectors from strategy's params: %+v", err)) } } if len(args.ExcludedTaints) > 0 && len(args.IncludedTaints) > 0 { - return fmt.Errorf("either includedTaints or excludedTaints can be set, but not both") + allErrs = append(allErrs, fmt.Errorf("either includedTaints or excludedTaints can be set, but not both")) } - return nil + return utilerrors.NewAggregate(allErrs) } diff --git a/pkg/framework/plugins/removepodsviolatingnodetaints/validation_test.go b/pkg/framework/plugins/removepodsviolatingnodetaints/validation_test.go index 8b83ba0ec..40bcfdaa3 100644 --- a/pkg/framework/plugins/removepodsviolatingnodetaints/validation_test.go +++ b/pkg/framework/plugins/removepodsviolatingnodetaints/validation_test.go @@ -1,6 +1,7 @@ package removepodsviolatingnodetaints import ( + "fmt" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -11,7 +12,7 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { testCases := []struct { description string args *RemovePodsViolatingNodeTaintsArgs - expectError bool + errInfo error }{ { description: "valid namespace args, no errors", @@ -20,7 +21,6 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { Include: []string{"default"}, }, }, - expectError: false, }, { description: "invalid namespaces args, expects error", @@ -30,7 +30,7 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { Exclude: []string{"kube-system"}, }, }, - expectError: true, + errInfo: fmt.Errorf(`only one of Include/Exclude namespaces can be set`), }, { description: "valid label selector args, no errors", @@ -39,7 +39,6 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { MatchLabels: map[string]string{"role.kubernetes.io/node": ""}, }, }, - expectError: false, }, { description: "invalid label selector args, expects errors", @@ -52,14 +51,13 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { }, }, }, - expectError: true, + errInfo: fmt.Errorf(`failed to get label selectors from strategy's params: [key: Invalid value: "": name part must be non-empty; name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]'), values: Invalid value: []string(nil): for 'in', 'notin' operators, values set can't be empty]`), }, { description: "valid taint filters, no errors", args: &RemovePodsViolatingNodeTaintsArgs{ ExcludedTaints: []string{"testTaint1=test1"}, }, - expectError: false, }, { description: "invalid taint filters args, expects errors", @@ -67,17 +65,19 @@ func TestValidateRemovePodsViolatingNodeTaintsArgs(t *testing.T) { ExcludedTaints: []string{"do-not-evict"}, IncludedTaints: []string{"testTaint1=test1"}, }, - expectError: true, + errInfo: fmt.Errorf(`either includedTaints or excludedTaints can be set, but not both`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemovePodsViolatingNodeTaintsArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemovePodsViolatingNodeTaintsArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) } diff --git a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go index f7f7f1164..b447c000f 100644 --- a/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go +++ b/pkg/framework/plugins/removepodsviolatingtopologyspreadconstraint/validation_test.go @@ -1,6 +1,7 @@ package removepodsviolatingtopologyspreadconstraint import ( + "fmt" "testing" v1 "k8s.io/api/core/v1" @@ -13,7 +14,7 @@ func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { testCases := []struct { description string args *RemovePodsViolatingTopologySpreadConstraintArgs - expectError bool + errInfo error }{ { description: "valid namespace args, no errors", @@ -22,7 +23,6 @@ func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { Include: []string{"default"}, }, }, - expectError: false, }, { description: "invalid namespaces args, expects error", @@ -32,7 +32,7 @@ func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { Exclude: []string{"kube-system"}, }, }, - expectError: true, + errInfo: fmt.Errorf(`only one of Include/Exclude namespaces can be set`), }, { description: "valid label selector args, no errors", @@ -41,7 +41,6 @@ func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { MatchLabels: map[string]string{"role.kubernetes.io/node": ""}, }, }, - expectError: false, }, { description: "invalid label selector args, expects errors", @@ -54,31 +53,32 @@ func TestValidateRemovePodsViolatingTopologySpreadConstraintArgs(t *testing.T) { }, }, }, - expectError: true, + errInfo: fmt.Errorf(`failed to get label selectors from strategy's params: [key: Invalid value: "": name part must be non-empty; name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]'), values: Invalid value: []string(nil): for 'in', 'notin' operators, values set can't be empty]`), }, { description: "valid constraints args, no errors", args: &RemovePodsViolatingTopologySpreadConstraintArgs{ Constraints: []v1.UnsatisfiableConstraintAction{v1.DoNotSchedule, v1.ScheduleAnyway}, }, - expectError: false, }, { description: "invalid constraints args, expects errors", args: &RemovePodsViolatingTopologySpreadConstraintArgs{ Constraints: []v1.UnsatisfiableConstraintAction{"foo"}, }, - expectError: true, + errInfo: fmt.Errorf(`constraint foo is not one of map[DoNotSchedule:{} ScheduleAnyway:{}]`), }, } - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - err := ValidateRemovePodsViolatingTopologySpreadConstraintArgs(tc.args) - - hasError := err != nil - if tc.expectError != hasError { - t.Error("unexpected arg validation behavior") + for _, testCase := range testCases { + t.Run(testCase.description, func(t *testing.T) { + validateErr := ValidateRemovePodsViolatingTopologySpreadConstraintArgs(testCase.args) + if validateErr == nil || testCase.errInfo == nil { + if validateErr != testCase.errInfo { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) + } + } else if validateErr.Error() != testCase.errInfo.Error() { + t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr) } }) }