Merge pull request #1729 from googs1025/validate/improve

Improves the validation logic in the plugins

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",
+				allErrs = append(allErrs, fmt.Errorf("invalid pod protection policy in ExtraEnabled: %q. Valid options are: %v",
-					string(policy), extraPodProtections)
+					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",
+				allErrs = append(allErrs, fmt.Errorf("invalid pod protection policy in DefaultDisabled: %q. Valid options are: %v",
-					string(policy), defaultPodProtections)
+					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 {

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 {

pkg/framework/plugins/podlifetime/validation.go

@@ -18,29 +18,31 @@ package podlifetime
 
 import (
 	"fmt"
-
+	"sort"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidatePodLifeTimeArgs(tc.args)
+			validateErr := ValidatePodLifeTimeArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemoveDuplicatesArgs(tc.args)
+			validateErr := ValidateRemoveDuplicatesArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemoveFailedPodsArgs(tc.args)
+			validateErr := ValidateRemoveFailedPodsArgs(testCase.args)
-			hasError := err != nil
+			if validateErr == nil || testCase.errInfo == nil {
-			if tc.expectError != hasError {
+				if validateErr != testCase.errInfo {
-				t.Error("unexpected arg validation behavior")
+					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)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemovePodsHavingTooManyRestartsArgs(tc.args)
+			validateErr := ValidateRemovePodsHavingTooManyRestartsArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemovePodsViolatingInterPodAntiAffinityArgs(tc.args)
+			validateErr := ValidateRemovePodsViolatingInterPodAntiAffinityArgs(testCase.args)
-			hasError := err != nil
+			if validateErr == nil || testCase.errInfo == nil {
-			if tc.expectError != hasError {
+				if validateErr != testCase.errInfo {
-				t.Error("unexpected arg validation behavior")
+					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)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemovePodsViolatingNodeAffinityArgs(tc.args)
+			validateErr := ValidateRemovePodsViolatingNodeAffinityArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}

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)
 }

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemovePodsViolatingNodeTaintsArgs(tc.args)
+			validateErr := ValidateRemovePodsViolatingNodeTaintsArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}

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 {
+	for _, testCase := range testCases {
-		t.Run(tc.description, func(t *testing.T) {
+		t.Run(testCase.description, func(t *testing.T) {
-			err := ValidateRemovePodsViolatingTopologySpreadConstraintArgs(tc.args)
+			validateErr := ValidateRemovePodsViolatingTopologySpreadConstraintArgs(testCase.args)
-
+			if validateErr == nil || testCase.errInfo == nil {
-			hasError := err != nil
+				if validateErr != testCase.errInfo {
-			if tc.expectError != hasError {
+					t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
-				t.Error("unexpected arg validation behavior")
+				}
+			} else if validateErr.Error() != testCase.errInfo.Error() {
+				t.Errorf("expected validity of plugin config: %q but got %q instead", testCase.errInfo, validateErr)
 			}
 		})
 	}