Merge pull request #45122 from ravisantoshgudimetla/priority_overflow#24720

Automatic merge from submit-queue (batch tested with PRs 44727, 45409, 44968, 45122, 45493)

Total priority overflow check

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #24720

**Special notes for your reviewer**:
@adohe. I have borrowed some parts of your code in the closed PR and created this one.

**Release note**:

```release-note
This fixes the overflow for priorityconfig-  valid range {1, 9223372036854775806}.
```

plugin/pkg/scheduler/api/types.go

@@ -24,6 +24,14 @@ import (
 	"k8s.io/kubernetes/pkg/api/v1"
 )
 
+const (
+	MaxUint          = ^uint(0)
+	MaxInt           = int(MaxUint >> 1)
+	MaxTotalPriority = MaxInt
+	MaxPriority      = 10
+	MaxWeight        = MaxInt / MaxPriority
+)
+
 type Policy struct {
 	metav1.TypeMeta
 	// Holds the information to configure the fit predicate functions

plugin/pkg/scheduler/api/validation/validation.go

@@ -29,8 +29,8 @@ func ValidatePolicy(policy schedulerapi.Policy) error {
 	var validationErrors []error
 
 	for _, priority := range policy.Priorities {
-		if priority.Weight <= 0 {
-			validationErrors = append(validationErrors, fmt.Errorf("Priority %s should have a positive weight applied to it", priority.Name))
+		if priority.Weight <= 0 || priority.Weight >= schedulerapi.MaxWeight {
+			validationErrors = append(validationErrors, fmt.Errorf("Priority %s should have a positive weight applied to it or it has overflown", priority.Name))
 		}
 	}
 

plugin/pkg/scheduler/api/validation/validation_test.go

@@ -51,6 +51,13 @@ func TestValidatePriorityWithNegativeWeight(t *testing.T) {
 	}
 }
 
+func TestValidatePriorityWithOverFlowWeight(t *testing.T) {
+	policy := api.Policy{Priorities: []api.PriorityPolicy{{Name: "WeightPriority", Weight: api.MaxWeight}}}
+	if ValidatePolicy(policy) == nil {
+		t.Errorf("Expected error about priority weight not being overflown.")
+	}
+}
+
 func TestValidateExtenderWithNonNegativeWeight(t *testing.T) {
 	extenderPolicy := api.Policy{ExtenderConfigs: []api.ExtenderConfig{{URLPrefix: "http://127.0.0.1:8081/extender", FilterVerb: "filter", Weight: 2}}}
 	errs := ValidatePolicy(extenderPolicy)

plugin/pkg/scheduler/factory/plugins.go

@@ -23,13 +23,12 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/golang/glog"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/predicates"
 	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/priorities"
 	schedulerapi "k8s.io/kubernetes/plugin/pkg/scheduler/api"
-
-	"github.com/golang/glog"
 )
 
 // PluginFactoryArgs are passed to all plugin factory functions.
@@ -356,9 +355,25 @@ func getPriorityFunctionConfigs(names sets.String, args PluginFactoryArgs) ([]al
 			})
 		}
 	}
+	if err := validateSelectedConfigs(configs); err != nil {
+		return nil, err
+	}
 	return configs, nil
 }
 
+// validateSelectedConfigs validates the config weights to avoid the overflow.
+func validateSelectedConfigs(configs []algorithm.PriorityConfig) error {
+	var totalPriority int
+	for _, config := range configs {
+		// Checks totalPriority against MaxTotalPriority to avoid overflow
+		if config.Weight*schedulerapi.MaxPriority > schedulerapi.MaxTotalPriority-totalPriority {
+			return fmt.Errorf("Total priority of priority functions has overflown")
+		}
+		totalPriority += config.Weight * schedulerapi.MaxPriority
+	}
+	return nil
+}
+
 var validName = regexp.MustCompile("^[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])$")
 
 func validateAlgorithmNameOrDie(name string) {

plugin/pkg/scheduler/factory/plugins_test.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package factory
 
-import "testing"
+import (
+	"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm"
+	"k8s.io/kubernetes/plugin/pkg/scheduler/api"
+	"testing"
+)
 
 func TestAlgorithmNameValidation(t *testing.T) {
 	algorithmNamesShouldValidate := []string{
@@ -39,3 +43,39 @@ func TestAlgorithmNameValidation(t *testing.T) {
 		}
 	}
 }
+
+func TestValidatePriorityConfigOverFlow(t *testing.T) {
+	tests := []struct {
+		description string
+		configs     []algorithm.PriorityConfig
+		expected    bool
+	}{
+		{
+			description: "one of the weights is MaxInt",
+			configs:     []algorithm.PriorityConfig{{Weight: api.MaxInt}, {Weight: 5}},
+			expected:    true,
+		},
+		{
+			description: "after multiplication with MaxPriority the weight is larger than MaxWeight",
+			configs:     []algorithm.PriorityConfig{{Weight: api.MaxInt/api.MaxPriority + api.MaxPriority}, {Weight: 5}},
+			expected:    true,
+		},
+		{
+			description: "normal weights",
+			configs:     []algorithm.PriorityConfig{{Weight: 10000}, {Weight: 5}},
+			expected:    false,
+		},
+	}
+	for _, test := range tests {
+		err := validateSelectedConfigs(test.configs)
+		if test.expected {
+			if err == nil {
+				t.Errorf("Expected Overflow for %s", test.description)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("Did not expect an overflow for %s", test.description)
+			}
+		}
+	}
+}