From a4907262455a7de0cd36c908dca319ce6e5eb06a Mon Sep 17 00:00:00 2001
From: Shubham Minglani <shubham@linux.com>
Date: Fri, 24 Nov 2017 18:47:10 +0530
Subject: [PATCH] Validate resource names to pass to thresholds

This commit adds checks to only allow valid resource names for the
thresholds field, which are "cpu", "memory", and "pods" right now.

For the other valid or invalid resource names, descheduler will now
throw an error.

Also, tests have been added to test the added behavior.

fix #40
---
 .../strategies/lownodeutilization.go          | 22 +++----
 .../strategies/lownodeutilization_test.go     | 57 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/pkg/descheduler/strategies/lownodeutilization.go b/pkg/descheduler/strategies/lownodeutilization.go
index bff65efe0..f4b514744 100644
--- a/pkg/descheduler/strategies/lownodeutilization.go
+++ b/pkg/descheduler/strategies/lownodeutilization.go
@@ -77,22 +77,24 @@ func LowNodeUtilization(ds *options.DeschedulerServer, strategy api.DeschedulerS
 }
 
 func validateThresholds(thresholds api.ResourceThresholds) bool {
-	if thresholds == nil {
+	if thresholds == nil || len(thresholds) == 0 {
 		glog.V(1).Infof("no resource threshold is configured")
 		return false
 	}
-	found := false
 	for name := range thresholds {
-		if name == v1.ResourceCPU || name == v1.ResourceMemory || name == v1.ResourcePods {
-			found = true
-			break
+		switch name {
+		case v1.ResourceCPU:
+			continue
+		case v1.ResourceMemory:
+			continue
+		case v1.ResourcePods:
+			continue
+		default:
+			glog.Errorf("only cpu, memory, or pods thresholds can be specified")
+			return false
 		}
 	}
-	if !found {
-		glog.V(1).Infof("one of cpu, memory, or pods resource threshold must be configured")
-		return false
-	}
-	return found
+	return true
 }
 
 //This function could be merged into above once we are clear.
diff --git a/pkg/descheduler/strategies/lownodeutilization_test.go b/pkg/descheduler/strategies/lownodeutilization_test.go
index e1f332c0a..1718b2c1a 100644
--- a/pkg/descheduler/strategies/lownodeutilization_test.go
+++ b/pkg/descheduler/strategies/lownodeutilization_test.go
@@ -18,6 +18,9 @@ package strategies
 
 import (
 	"fmt"
+	"strings"
+	"testing"
+
 	"github.com/kubernetes-incubator/descheduler/pkg/api"
 	"github.com/kubernetes-incubator/descheduler/test"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -25,8 +28,6 @@ import (
 	core "k8s.io/client-go/testing"
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake"
-	"strings"
-	"testing"
 )
 
 // TODO: Make this table driven.
@@ -109,3 +110,55 @@ func TestLowNodeUtilization(t *testing.T) {
 	}
 
 }
+
+func TestValidateThresholds(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   api.ResourceThresholds
+		succeed bool
+	}{
+		{
+			name:    "passing nil map for threshold",
+			input:   nil,
+			succeed: false,
+		},
+		{
+			name:    "passing no threshold",
+			input:   api.ResourceThresholds{},
+			succeed: false,
+		},
+		{
+			name: "passing unsupported resource name",
+			input: api.ResourceThresholds{
+				v1.ResourceCPU:     40,
+				v1.ResourceStorage: 25.5,
+			},
+			succeed: false,
+		},
+		{
+			name: "passing invalid resource name",
+			input: api.ResourceThresholds{
+				v1.ResourceCPU: 40,
+				"coolResource": 42.0,
+			},
+			succeed: false,
+		},
+		{
+			name: "passing a valid threshold with cpu, memory and pods",
+			input: api.ResourceThresholds{
+				v1.ResourceCPU:    20,
+				v1.ResourceMemory: 30,
+				v1.ResourcePods:   40,
+			},
+			succeed: true,
+		},
+	}
+
+	for _, test := range tests {
+		isValid := validateThresholds(test.input)
+
+		if isValid != test.succeed {
+			t.Errorf("expected validity of threshold: %#v\nto be %v but got %v instead", test.input, test.succeed, isValid)
+		}
+	}
+}