From b8394c4700acb85cb9b5b38d188a6b5438769e67 Mon Sep 17 00:00:00 2001
From: Dave Chen <dave.jungler@gmail.com>
Date: Fri, 5 Mar 2021 17:55:09 +0800
Subject: [PATCH] Move VolumeBinding plugin args validation to
 apis/config/validation

This PR also looses the check to allow zero since the API doc has
explained that value zero indicates no waiting.

Signed-off-by: Dave Chen <dave.chen@arm.com>
---
 .../validation/validation_pluginargs.go       | 12 ++++++
 .../validation/validation_pluginargs_test.go  | 39 +++++++++++++++++++
 .../plugins/volumebinding/volume_binding.go   | 10 +----
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go
index 8773b39dcfd6e..182f616bf102b 100644
--- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go
+++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go
@@ -298,3 +298,15 @@ func ValidateNodeAffinityArgs(args *config.NodeAffinityArgs) error {
 	}
 	return errors.Flatten(errors.NewAggregate(errs))
 }
+
+// ValidateVolumeBindingArgs validates that VolumeBindingArgs are set correctly.
+func ValidateVolumeBindingArgs(args *config.VolumeBindingArgs) error {
+	var path *field.Path
+	var err error
+
+	if args.BindTimeoutSeconds < 0 {
+		err = field.Invalid(path.Child("bindTimeoutSeconds"), args.BindTimeoutSeconds, "invalid BindTimeoutSeconds, should not be a negative value")
+	}
+
+	return err
+}
diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go
index 5ca7bc328f059..1c9d5c922beec 100644
--- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go
+++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go
@@ -945,3 +945,42 @@ func TestValidateNodeAffinityArgs(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateVolumeBindingArgs(t *testing.T) {
+	cases := []struct {
+		name    string
+		args    config.VolumeBindingArgs
+		wantErr error
+	}{
+		{
+			name: "zero is a valid config",
+			args: config.VolumeBindingArgs{
+				BindTimeoutSeconds: 0,
+			},
+		},
+		{
+			name: "positive value is valid config",
+			args: config.VolumeBindingArgs{
+				BindTimeoutSeconds: 10,
+			},
+		},
+		{
+			name: "negative value is invalid config ",
+			args: config.VolumeBindingArgs{
+				BindTimeoutSeconds: -10,
+			},
+			wantErr: &field.Error{
+				Type:  field.ErrorTypeInvalid,
+				Field: "bindTimeoutSeconds",
+			},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateVolumeBindingArgs(&tc.args)
+			if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" {
+				t.Errorf("ValidateVolumeBindingArgs returned err (-want,+got):\n%s", diff)
+			}
+		})
+	}
+}
diff --git a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go
index 19d2067696212..144790310328c 100644
--- a/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go
+++ b/pkg/scheduler/framework/plugins/volumebinding/volume_binding.go
@@ -32,6 +32,7 @@ import (
 	"k8s.io/kubernetes/pkg/controller/volume/scheduling"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
+	"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
 	"k8s.io/kubernetes/pkg/scheduler/framework"
 )
 
@@ -286,7 +287,7 @@ func New(plArgs runtime.Object, fh framework.Handle) (framework.Plugin, error) {
 	if !ok {
 		return nil, fmt.Errorf("want args to be of type VolumeBindingArgs, got %T", plArgs)
 	}
-	if err := validateArgs(args); err != nil {
+	if err := validation.ValidateVolumeBindingArgs(args); err != nil {
 		return nil, err
 	}
 	podInformer := fh.SharedInformerFactory().Core().V1().Pods()
@@ -309,10 +310,3 @@ func New(plArgs runtime.Object, fh framework.Handle) (framework.Plugin, error) {
 		GenericEphemeralVolumeFeatureEnabled: utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume),
 	}, nil
 }
-
-func validateArgs(args *config.VolumeBindingArgs) error {
-	if args.BindTimeoutSeconds <= 0 {
-		return fmt.Errorf("invalid BindTimeoutSeconds: %d, must be positive integer", args.BindTimeoutSeconds)
-	}
-	return nil
-}