Merge pull request #512 from PiotrProkop/nrt-add-validation

Introduce pluginargs validation for NodeResourceTopologyMatch plugin

Author: k8s-ci-robot

apis/config/validation/validation_pluginargs.go

@@ -0,0 +1,48 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	"sigs.k8s.io/scheduler-plugins/apis/config"
+)
+
+var validScoringStrategy = sets.NewString(
+	string(config.MostAllocated),
+	string(config.BalancedAllocation),
+	string(config.LeastAllocated),
+	string(config.LeastNUMANodes),
+)
+
+func ValidateNodeResourceTopologyMatchArgs(path *field.Path, args *config.NodeResourceTopologyMatchArgs) error {
+	var allErrs field.ErrorList
+	scoringStrategyTypePath := path.Child("scoringStrategy.type")
+	if err := validateScoringStrategyType(args.ScoringStrategy.Type, scoringStrategyTypePath); err != nil {
+		allErrs = append(allErrs, err)
+	}
+
+	return allErrs.ToAggregate()
+}
+
+func validateScoringStrategyType(scoringStrategy config.ScoringStrategyType, path *field.Path) *field.Error {
+	if !validScoringStrategy.Has(string(scoringStrategy)) {
+		return field.Invalid(path, scoringStrategy, "invalid ScoringStrategyType")
+	}
+	return nil
+}

apis/config/validation/validation_plugingargs_test.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"sigs.k8s.io/scheduler-plugins/apis/config"
+)
+
+func TestValidateNodeResourceTopologyMatchArgs(t *testing.T) {
+	testCases := []struct {
+		args        *config.NodeResourceTopologyMatchArgs
+		expectedErr error
+		description string
+	}{
+		{
+			description: "correct config",
+			args: &config.NodeResourceTopologyMatchArgs{
+				ScoringStrategy: config.ScoringStrategy{
+					Type: config.MostAllocated,
+				},
+			},
+		},
+		{
+			description: "incorrect config, wrong ScoringStrategy type",
+			args: &config.NodeResourceTopologyMatchArgs{
+				ScoringStrategy: config.ScoringStrategy{
+					Type: "not existent",
+				},
+			},
+			expectedErr: fmt.Errorf("scoringStrategy.type: Invalid value:"),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			err := ValidateNodeResourceTopologyMatchArgs(nil, testCase.args)
+			if testCase.expectedErr != nil {
+				if err == nil {
+					t.Errorf("expected err to equal %v not nil", testCase.expectedErr)
+				}
+
+				if !strings.Contains(err.Error(), testCase.expectedErr.Error()) {
+					t.Errorf("expected err to contain %s in error message: %s", testCase.expectedErr.Error(), err.Error())
+				}
+			}
+			if testCase.expectedErr == nil && err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+		})
+	}
+}

pkg/noderesourcetopology/plugin.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/kubernetes/pkg/scheduler/framework"
 
 	apiconfig "sigs.k8s.io/scheduler-plugins/apis/config"
+	"sigs.k8s.io/scheduler-plugins/apis/config/validation"
 	nrtcache "sigs.k8s.io/scheduler-plugins/pkg/noderesourcetopology/cache"
 
 	topologyapi "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology"
@@ -120,6 +121,10 @@ func New(args runtime.Object, handle framework.Handle) (framework.Plugin, error)
 		return nil, fmt.Errorf("want args to be of type NodeResourceTopologyMatchArgs, got %T", args)
 	}
 
+	if err := validation.ValidateNodeResourceTopologyMatchArgs(nil, tcfg); err != nil {
+		return nil, err
+	}
+
 	nrtCache, err := initNodeTopologyInformer(tcfg, handle)
 	if err != nil {
 		klog.ErrorS(err, "Cannot create clientset for NodeTopologyResource", "kubeConfig", handle.KubeConfig())

test/integration/noderesourcetopology_test.go

@@ -27,14 +27,18 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/dynamic/dynamicinformer"
 	"k8s.io/client-go/kubernetes"
 	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/tools/events"
 	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/pkg/scheduler"
 	schedapi "k8s.io/kubernetes/pkg/scheduler/apis/config"
 	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/defaultbinder"
 	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/queuesort"
 	fwkruntime "k8s.io/kubernetes/pkg/scheduler/framework/runtime"
+	"k8s.io/kubernetes/pkg/scheduler/profile"
 	st "k8s.io/kubernetes/pkg/scheduler/testing"
 	imageutils "k8s.io/kubernetes/test/utils/image"
 
@@ -76,6 +80,48 @@ type nrtTestEntry struct {
 	errMsg                 string
 }
 
+func TestTopologyMatchPluginValidation(t *testing.T) {
+	cfg, err := util.NewDefaultSchedulerComponentConfig()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg.Profiles[0].Plugins.Filter.Enabled = append(cfg.Profiles[0].Plugins.Filter.Enabled, schedapi.Plugin{Name: noderesourcetopology.Name})
+	cfg.Profiles[0].Plugins.Score.Enabled = append(cfg.Profiles[0].Plugins.Score.Enabled, schedapi.Plugin{Name: noderesourcetopology.Name})
+	cfg.Profiles[0].PluginConfig = append(cfg.Profiles[0].PluginConfig, schedapi.PluginConfig{
+		Name: noderesourcetopology.Name,
+		Args: &scheconfig.NodeResourceTopologyMatchArgs{
+			ScoringStrategy: scheconfig.ScoringStrategy{Type: "incorrect"},
+		},
+	})
+
+	cs := kubernetes.NewForConfigOrDie(globalKubeConfig)
+	informer := scheduler.NewInformerFactory(cs, 0)
+	dynInformerFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(dynamic.NewForConfigOrDie(globalKubeConfig), 0, v1.NamespaceAll, nil)
+	eventBroadcaster := events.NewBroadcaster(&events.EventSinkImpl{
+		Interface: cs.EventsV1(),
+	})
+	done := make(chan struct{})
+
+	_, err = scheduler.New(
+		cs,
+		informer,
+		dynInformerFactory,
+		profile.NewRecorderFactory(eventBroadcaster),
+		done,
+		scheduler.WithProfiles(cfg.Profiles...),
+		scheduler.WithFrameworkOutOfTreeRegistry(fwkruntime.Registry{noderesourcetopology.Name: noderesourcetopology.New}),
+	)
+
+	if err == nil {
+		t.Error("expected error not to be nil")
+	}
+
+	errMsg := "scoringStrategy.type: Invalid value:"
+	if !strings.Contains(err.Error(), errMsg) {
+		t.Errorf("expected error to contain: %s in error message: %s", errMsg, err.Error())
+	}
+}
+
 func TestTopologyMatchPlugin(t *testing.T) {
 	testCtx := &testContext{}
 	testCtx.Ctx, testCtx.CancelFn = context.WithCancel(context.Background())